Page MenuHomePhabricator

Remove StreamConfig::INTERNAL_SETTINGS logic from EventStreamConfig and do it in EventLogging client instead
Open, HighPublic

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

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-Planning 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()