Page MenuHomePhabricator

Remove StreamConfig::INTERNAL_SETTINGS logic from EventStreamConfig and do it in EventLogging client instead
Closed, ResolvedPublic

Description

The EventLogging MediaWiki extension requests stream configs for many streams on every page load. To keep the payload size down, we excluded extraneous stream config settings by default via the StreamConfig::INTERNAL_SETTINGS const. This is a bit hacky, as it hardcodes some settings in the extension code itself.

Stream Config is beginning to be used by more clients, and more settings are being added, e.g. https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/668124.

Since the reason for automatically excluding certain settings is really just for the EventLogging on every page load use case, we should make the EventLogging + EventStreamConfig + ResourceLoader bridge do the filtering of settings for EventLogging. That way, EventLogging can explictly set what settings should be exported.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
odimitrijevic moved this task from Incoming to Event Platform on the Analytics board.

I was about to write a task to request that consumer is also removed from the stream configs that are included in the ext.eventLogging RL module as its extraneous and increases payload size.

I'd also suggest that we shift to allowlisting stream config settings sent as part of the payload. An allowlist would be easier to maintain and would be more resilient to changes in the shape of the underlying stream configs, however infrequent.

I'd also suggest that we shift to allowlisting stream config settings sent as part of the payload

+1

Change 831084 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/EventLogging@master] Hooks: Filter stream config settings

https://gerrit.wikimedia.org/r/831084

Change 831129 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/EventStreamConfig@master] StreamConfig: Soft deprecate parameter

https://gerrit.wikimedia.org/r/831129

Change 831130 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/EventStreamConfig@master] Api: Do not pass deprecated parameter

https://gerrit.wikimedia.org/r/831130

Change 831131 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/EventStreamConfig@master] StreamConfigs: Hard deprecate parameter

https://gerrit.wikimedia.org/r/831131

Change 831132 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/EventBus@master] EventBusFactory: Do not pass deprecated parameter

https://gerrit.wikimedia.org/r/831132

Change 831130 abandoned by Phuedx:

[mediawiki/extensions/EventStreamConfig@master] Api: Do not pass deprecated parameter

Reason:

See I83429bc3238.

https://gerrit.wikimedia.org/r/831130

Change 831129 abandoned by Phuedx:

[mediawiki/extensions/EventStreamConfig@master] StreamConfig: Soft deprecate parameter

Reason:

See I83429bc3238.

https://gerrit.wikimedia.org/r/831129

Change 831129 restored by Phuedx:

[mediawiki/extensions/EventStreamConfig@master] StreamConfig: Soft deprecate parameter

https://gerrit.wikimedia.org/r/831129

Change 831236 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/EventLogging@master] EventLogging: Do not pass deprecated parameter

https://gerrit.wikimedia.org/r/831236

Great stuff! TY Sam!

I think there are action API usages out there that set all_settings=true, but IIUC that param will just be ignored now, and all settings will always be returned, right?

I think there are action API usages out there that set all_settings=true, but IIUC that param will just be ignored now, and all settings will always be returned, right?

Here's a list of those action API usages: https://codesearch.wmcloud.org/search/?q=all_settings%5C%3D(1%7Ctrue). There don't appear to be any usages where all_settings is falsy: https://codesearch.wmcloud.org/search/?q=all_settings%5C%3D(0%7Cfalse).

I would recommend updating those usages before merging StreamConfigs: Hard deprecate parameter, which is the last patch in the relation chain.

Status update:

@Ottomata has signed off on (+1'd) all of the patches in the relation chain. Metrics Platform Backlog is responsible for getting them deployed. We'll do so in two stages to ease monitoring each deployment:

Stage 1: Deploy the patch that filters stream config settings in EventLogging
Stage 2: Deploy the patches that deprecate the $includeAllSettings parameter for StreamConfigs::get()

Change 831084 merged by jenkins-bot:

[mediawiki/extensions/EventLogging@master] Hooks: Filter stream config settings

https://gerrit.wikimedia.org/r/831084

Change 831129 merged by jenkins-bot:

[mediawiki/extensions/EventStreamConfig@master] StreamConfig: Soft deprecate parameter

https://gerrit.wikimedia.org/r/831129

Change 831236 merged by jenkins-bot:

[mediawiki/extensions/EventLogging@master] EventLogging: Do not pass deprecated parameter

https://gerrit.wikimedia.org/r/831236

Change 831132 merged by jenkins-bot:

[mediawiki/extensions/EventBus@master] EventBusFactory: Do not pass deprecated parameter

https://gerrit.wikimedia.org/r/831132

Change 902075 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/EventBus@master] EventBusFactory: Do not pass deprecated parameter

https://gerrit.wikimedia.org/r/902075

Change 902075 merged by jenkins-bot:

[mediawiki/extensions/EventBus@master] EventBusFactory: Do not pass deprecated parameter

https://gerrit.wikimedia.org/r/902075

Change 902118 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/libs/metrics-platform@master] [Java][Swift] Remove all_settings param from API call

https://gerrit.wikimedia.org/r/902118

Change 902118 merged by jenkins-bot:

[mediawiki/libs/metrics-platform@master] [Java][Swift] Remove all_settings param from API call

https://gerrit.wikimedia.org/r/902118

Change 831131 merged by jenkins-bot:

[mediawiki/extensions/EventStreamConfig@master] StreamConfigs: Hard deprecate parameter

https://gerrit.wikimedia.org/r/831131

🎉

We'll monitor logstash as the hard deprecation rolls out next week.

I'll check back in on the dashboard above on Monday 17th.

Change 908251 had a related patch set uploaded (by Ottomata; author: Ottomata):

[operations/deployment-charts@master] eventgate - remove deprecated all_settings stream config param

https://gerrit.wikimedia.org/r/908251

Change 908251 merged by Ottomata:

[operations/deployment-charts@master] eventgate - remove deprecated all_settings stream config param

https://gerrit.wikimedia.org/r/908251

Change 910046 had a related patch set uploaded (by Phuedx; author: Phuedx):

[analytics/refinery@master] Remove deprecated all_settings streamconfigs param

https://gerrit.wikimedia.org/r/910046

Change 910046 merged by Ottomata:

[analytics/refinery@master] Remove deprecated all_settings streamconfigs param

https://gerrit.wikimedia.org/r/910046

@Ottomata: Thanks for merging that patch. There are a couple of mentions remaining, which look like they need to be updated. Is that right?

Looks right! All of those can be safely changed.

Change 910470 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/services/eventstreams@master] Remove deprecated all_settings streamconfigs param

https://gerrit.wikimedia.org/r/910470

Change 910471 had a related patch set uploaded (by Phuedx; author: Phuedx):

[operations/deployment-charts@master] Remove deprecated all_settings streamconfigs param

https://gerrit.wikimedia.org/r/910471

Change 910470 merged by jenkins-bot:

[mediawiki/services/eventstreams@master] Remove deprecated all_settings streamconfigs param

https://gerrit.wikimedia.org/r/910470

@Ottomata: I don't appear to have permissions to edit the Puppet config for deployment-eventstreams-2.deployment-prep on Horizon. Do you?

Change 910471 abandoned by Ottomata:

[operations/deployment-charts@master] Remove deprecated all_settings streamconfigs param

Reason:

Done in a different patch

https://gerrit.wikimedia.org/r/910471