Page MenuHomePhabricator

Provide a reusable getEditCountBucket function for analytics purposes
Closed, ResolvedPublic

Description

Bucketing users into edit-count buckets is a strategy used by QuickSurveys, Popups

https://github.com/search?q=org%3Awikimedia+100-999&type=Code
We would also like to use it in Minerva in MobileWebMainMenuClickTracking

EventLogging is always involved.

Acceptance Criteria

To support this

  • We should add the get getEditCountBucket method to WikimediaEvents OR core OR EventLogging
  • Update QuickSurveys to use the new method
  • Do not change the buckets. Changing the user buckets (or adding to them is out of scope for this task). As soon as this is defined in one place, we can consider that if needed in a follow up.

Notes

  • We will not update Popups to use the new method because we are going to remove the code in T267211.
  • Minerva's MobileWebMainMenuClickTracking was moved to WikimediaEvents (modules/ext.wikimediaEvents/mobileWebUIActions.js )

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson changed the task status from Stalled to Open.Feb 25 2021, 3:45 PM

@phuedx should we call this resolved? It seems we are settling on thr WikimediaEvents defined schema and moving all schemas into that extension.

phuedx claimed this task.

Yes! Thanks for the ping, @Jdlrobson.

Sorry I wasn't aware of this task until now, and may have duplicated some work. In patch aca489f139cd13df, I added code to the EventLogging extension which makes wgUserEditCountBucket available from JS, and EventLoggingServices::getInstance()->getUserBucketProvider()->getUserEditCountBucket( $user ) from the backend.

Sorry I wasn't aware of this task until now, and may have duplicated some work. In patch aca489f139cd13df, I added code to the EventLogging extension which makes wgUserEditCountBucket available from JS, and EventLoggingServices::getInstance()->getUserBucketProvider()->getUserEditCountBucket( $user ) from the backend.

Neato!

Reopening this task as it feels like removing mw.wikimediaEvents.getUserEditCountBucket() is now in scope.

Given it relates to our existing language instrumentation pulling into upcoming.

I'm actually going to move this one back into Needs Analysis. AFAICT @awight's approach in rEEVLaca489f139cd: User edit count bucketing is one @Krinkle advised against in T210106#5829916 so we should probably chat about it.

Jdlrobson removed the point value for this task.Mar 2 2021, 5:28 PM

@Krinkle is this something you feel strongly about still? Happy to setup a sync between the 4 of us if so.

@awight's approach in rEEVLaca489f139cd: User edit count bucketing is one @Krinkle advised against in T210106#5829916 so we should probably chat about it.

I think we can do better than sending it in the <head>, which meanas it has a priority higher than that of the article content itself, and done unconditionally and upfront for all logged-in users on the server-side.

If we're confident that the nature of the bucketing is very unlikely to change, and that indeed there are no current use cases for two features to do it differently, and that we have a strategy in place for how we could change it, then perhaps it would make sense to add to EventLogging as client-side function, similar to what we have for inSample and pageViewToken.

This is more suited than WikimediaEvents since that is not something we can call methods from in other extensions (WMF-specific dependency), and is also more suited than something in core like mw.user since is much more prominent and even harder to change in terms of compat, and because almost by definition that means we waste more bytes since we know it's only needed when logging events, so shipping with EventLogging makes a lot of sense.

To indicate the compat complexity here, below is an example of what it might look like when changing it. I'm curious if there's a simpler way, or if this would be acceptable as-is?

  • Find every caller to the function.
  • For each caller, finds its extension and associated schema name (might be relatively indirect or deep in the code base).
  • For each of those EL schemas, make a Meta-Wiki edit to the schema such that it will support both current values and the new value(s) you intend to add to this function.
  • For each of the EventGate schemas, draft a commit that does this and get it reviewed, and then wait for deployment.
  • For each caller, make a commit in their respective MW extension repos updating to the new schema version.
  • Then, and only then, edit the function in the EventLogging repo to return the new values.
  • (Optional: Do the above again, but removing any now-obsolete enum values.)
  • (Ceveat: If one team prefers to keep the old, or if they are in a third-party repo; they will be broken in a surprising way, unless they copy the previous version of the function into their codebase. There is no easy way to have a deprecation warning or migration period unless we rename the function at the same time to force an error).

The above is why I recommended against it, but I suppose it's not very different from what we do with sessionId and pageViewToken. It's just that those don't have strict schema validation coupled with them (like enum). But if the above is fine with everyone, then I suppose mw.eventLog is the most appropriate place for it.

Hi @Krinkle, great to have your thoughts here!

I think we can do better than sending it in the <head>, which means it has a priority higher than that of the article content itself, and done unconditionally and upfront for all logged-in users on the server-side.

I hadn't thought about the <head> issue until now, it seems to be commonly misunderstood. It would be helpful to document the performance implications of MakeGlobalVariablesScript on the manual page perhaps. Looking in the <head>, I see that my team is already responsible for at least four other variables—impacting anonymous users as well!—and the variable names alone add up to 128 bytes (created task T276716 for work to fix this).

If we're confident that the nature of the bucketing is very unlikely to change, and that indeed there are no current use cases for two features to do it differently, and that we have a strategy in place for how we could change it, then perhaps it would make sense to add to EventLogging as client-side function, similar to what we have for inSample and pageViewToken.

Yes, I can confirm that all features are currently using the same user edit count bucketing thresholds. At first, I intended to provide the bucketing as a service which could be overridden by site service wiring configuration, but I suppose I dropped that: https://github.com/wikimedia/mediawiki-extensions-EventLogging/blob/master/includes/EventLoggingHooks.php#L43 . As you point out, changing the thresholds is unpleasant so I don't expect we do this often.

The migration you describe looks right, but I don't see it as formidable. The same complexity is present now, just with different timing requirements.

There's also a privacy benefit to consolidation: if two extensions were to have different thresholds for edit count bucketing, it would allow for finer-granularity deanonymization from supposedly sanitized data, because the intersections of the two edit count bucket threshold sets would be smaller than intended.

It looks like the action required is simply to move from a server-side calculation by the EventLogging extension to a client-side function. This will eliminate the <head> injection. The direct coupling is unfortunate—currently the global variable and mw.track are soft-coupled and gracefully do nothing if EventLogging isn't installed—but we'll just guard with if ( mw.eventLog !== undefined ) event.editCountBucket = mw.eventLog.getUserEditCountBucket().

Making that change, to move the server-side calculation entirely to JS. I would love any thoughts people have about how to prevent coupling.

Change 670146 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/EventLogging@master] Stop sending edit count bucket as a config var

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

awight moved this task from Doing to Review on the WMDE-TechWish-Sprint-2021-03-03 board.

Change 670426 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/EventLogging@master] [DNM] Stop sending edit count bucket as a config var

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

I would love any thoughts people have about how to prevent coupling.

Ideally this information (and other information, e.g. web_session_id and web_pageview_id in the web_identifiers fragment) would be filled in by EventLogging at the point the event is being prepared to be logged. This would increase the complexity of the EventLogging client but greatly reduce the complexity of our numerous instruments, reduce API binding between them and EventLogging, and allow centralised management of these critical fragments.

It looks like the action required is simply to move from a server-side calculation by the EventLogging extension to a client-side function. This will eliminate the <head> injection. The direct coupling is unfortunate—currently the global variable and mw.track are soft-coupled and gracefully do nothing if EventLogging isn't installed—but we'll just guard with if ( mw.eventLog !== undefined ) event.editCountBucket = mw.eventLog.getUserEditCountBucket().

You could have the new code in EventLogging also set a config variable like the server-side code that it replaced:

WikimediaEvents/modules/ext.eventLogging/core.js
// ...
mw.eventLog = {
  // ...
  getUserEditCountBucket: function () {
    // ...
  }
};

mw.config.set( 'wgUserEditCountBucket', mw.eventLog.getUserEditCountBucket() );

If the value is set, then clients can use it. Clients can remain bound to the EventLogging protocol and not to its API.

However there's a risk that your instrument executes before the ext.eventLogging module, which would leave the event.userEditCount property as undefined. Note well that this is also an issue with your proposed solution.

You could have the new code in EventLogging also set a config variable like the server-side code that it replaced:

This is a great suggestion, I'll try it out. I have some remorse about adding code that runs unconditionally, but it's cheap logic and we can hope will eventually be replaced by something like addRequestedValues transparent injection.

However there's a risk that your instrument executes before the ext.eventLogging module, which would leave the event.userEditCount property as undefined. Note well that this is also an issue with your proposed solution.

+1 the ordering thing is a big problem. I can only imagine extreme workarounds:

  • ResourceLoader introduces an "optional dependency" mechanism to order modules if they will be loaded, but without strictly requiring the dependencies to be present.
  • Do something dynamic in producing extensions, where we test for EventLogging and if present, add the eventlogging RL module as a dependency.
  • EventLogging initialization happens during the wikipage_content hook and producers cannot assume that the variable is available until after that. But that's useless for "page loaded" events.

Change 670426 abandoned by Awight:
[mediawiki/extensions/EventLogging@master] [DNM] Stop sending edit count bucket as a config var

Reason:
Decided to continue using the global variable, but change how it's initialized.

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

Change 670146 merged by jenkins-bot:
[mediawiki/extensions/EventLogging@master] Provide user edit count bucket as a function

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

What is the motivation for moving the mw.config.set instruction (presumably temporaily) to the client-side? This way indeed means that users of mw.track('event.•') will find it is now undefined. If we're okay with such direct breakage, might as well remove it entirely now and replace the callers before Tuesday morning for the next branch cut.

If all users we care about do have an explicit dependency on ext.eventLogging then I guess this is fine, though. Just to confirm though, we'll remove it in a short time once replacements have been made?

  • ResourceLoader introduces an "optional dependency" mechanism to order modules if they will be loaded, but without strictly requiring the dependencies to be present.
  • Do something dynamic in producing extensions, where we test for EventLogging and if present, add the eventlogging RL module as a dependency.

This is what mw.track() and mw.hook() are for.

  • EventLogging initialization happens during the wikipage_content hook and producers cannot assume that the variable is available until after that. But that's useless for "page loaded" events.

ext.eventLogging is loaded unconditionally on all page loads. Its API is small and conistently available even when e.g. a wiki does not have EL enabled or set up (e.g. a dev wiki or third-party wiki without EventGate). You can and should depend on this module explicitly in extensions if you use any of its method or variables, beyond the passive dep-free mw.track('event.•') API which indeed does not require such dependency. This naturally takes care of the ordering issue without adding cost since it's already unconditionally loaded, so this dependency is only for the execution order.

This does not apply to WikimediaEvents since that's WMF-specific and may not be depended on from other extensions.

This is what mw.track() and mw.hook() are for.

Exactly, and I'd like to not introduce any additional coupling, the code I've pushed so far is just a workaround. Since tracking the user's edit count bucket is normally not a responsibility of the event producer, it would ideally be transparent. See the proposed solution in T210106#6904031 , which would transparently inject metadata like this alongside skin identifier, etc., for event streams which "ask" to include the data.

  • EventLogging initialization happens during the wikipage_content hook and producers cannot assume that the variable is available until after that. But that's useless for "page loaded" events.

ext.eventLogging is loaded unconditionally on all page loads. Its API is small and conistently available even when e.g. a wiki does not have EL enabled or set up (e.g. a dev wiki or third-party wiki without EventGate). You can and should depend on this module explicitly in extensions if you use any of its method or variables, beyond the passive dep-free mw.track('event.•') API which indeed does not require such dependency. This naturally takes care of the ordering issue without adding cost since it's already unconditionally loaded, so this dependency is only for the execution order.

If a third-party wiki doesn't have EventLogging installed, then an extension front-end module depending on ext.eventLogging would fail to load. This is the case I have in mind when discussing this hypothetical, optional ResourceLoader dependency mechanism. Again, the dependency problem goes away completely if we declaratively request metadata injection and if EventLogging or EventGate is present, it can fulfill these directives transparently.

What is the motivation for moving the mw.config.set instruction (presumably temporaily) to the client-side? This way indeed means that users of mw.track('event.•') will find it is now undefined. If we're okay with such direct breakage, might as well remove it entirely now and replace the callers before Tuesday morning for the next branch cut.

If all users we care about do have an explicit dependency on ext.eventLogging then I guess this is fine, though. Just to confirm though, we'll remove it in a short time once replacements have been made?

The motivation for moving mw.config.set to the client-side was to avoid polluting the time-critical <head> element, moving it out of the MakeGlobalVariablesScript hook. Ambiguous dependency ordering does feel like a blocker. I don't see what else to do, though.

The callers don't currently have an explicit dependency on ext.eventLogging, and adding that would be the most "politically" expensive option since it might be a breaking change for many third-party installs. I'm planning to just cross my fingers, wait to see if the race conditions are in our favor, and in a couple of months switch to transparent injection once it's ready... I would love to do this better, but am out of ideas for the moment.

@Krinkle Here's a dirty idea: What if the producer code in each extension depends on a small, dynamic module registered using ResourceLoaderRegisterModules, which will contain { dependencies: 'ext.eventLogging' } when the EventLogging extension is present and enabled, and {} otherwise? This would be a temporary situation of course, hopefully << 1 year.

@Krinkle Here's a dirty idea: What if the producer code in each extension depends on a small, dynamic module registered using ResourceLoaderRegisterModules, which will contain { dependencies: 'ext.eventLogging' } when the EventLogging extension is present and enabled, and {} otherwise? This would be a temporary situation of course, hopefully << 1 year.

*sigh* I guess this is no good either, because it adds a c. 13-byte critical-path overhead for each of these modules.

You can and should depend on ext.eventLogging directly and unconditionally from any module that needs to call mw.eventLog methods. We do this in various features already. It's a very small extension with no runtime requirements or anything. It just provides this interface and does nothing by default if you have no related server set up.

In the common case, even that can be avoided by using mw.track('event.MySchema', …) instead of calling mw.eventLog directly. But for some of its util methods it can be called in that way indeed.

Change 678293 had a related patch set uploaded (by Awight; author: Awight):

[analytics/refinery/source@master] New UDF to normalize edit count bucket

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

You can and should depend on ext.eventLogging directly and unconditionally from any module that needs to call mw.eventLog methods.

That would be perfect for extensions which already depend on the EventLogging extension, but I'm very hesitant to create that high-level dependency unless necessary. The current implementation provided in this task lets logging stay loosely coupled through mw.track. The medium-term plan (IMHO) is that the global edit count bucket variable is deprecated by a transparent injection mechanism within EventGate.

I'll go ahead and migrate the existing edit count bucket usages, hopefully that offers enough savings in ES code removed that the impact of my changes are a net positive, even in this transitional state.

I'll go ahead and migrate the existing edit count bucket usages, hopefully that offers enough savings in ES code removed that the impact of my changes are a net positive, even in this transitional state.

Sorry, too much time passed and I lost sight of the problem here: if the eventLogging ResourceLoader dependency is not made explicit, then we have no guarantee that the bucket variable will be available by the time an event is constructed. So I'll only migrate the safe usages for now, where we declare the explicit dependency.

Change 678301 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/WikimediaEvents@master] Use edit count bucketing from EventLogging

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

Change 678302 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/QuickSurveys@master] Use edit count bucketing from EventLogging

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

The one remaining implementation is Popups/src/counts.js, but this has the soft-coupling, dependency race issue we've been discussing so I won't port it to rely on the injected global.

Change 678301 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] Use edit count bucketing from EventLogging

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

The one remaining implementation is Popups/src/counts.js, but this has the soft-coupling, dependency race issue we've been discussing so I won't port it to rely on the injected global.

Also queued for removal in T267211

I think this can be resolved now?

Change 678302 merged by jenkins-bot:

[mediawiki/extensions/QuickSurveys@master] Use edit count bucketing from EventLogging

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

Change 680180 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/WikimediaEvents@master] Omit userEditCountBucket field for anons and when unavailable

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

Change 680192 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/QuickSurveys@master] Also omit the edit count bucket when it's unavailable

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

@Krinkle wrote,

Broke searchSatisfaction (T280294). Please check if others may be broken as well.

Indeed, I made the same mistake everywhere. Also, it turns out there's a slight mismatch between the way the config variable works (falling back to null) and the way eventlogging validates an omitted field differently than a field with null value. Because of this, the conditional assignment needs to be pushed up to only assign a bucket when we know it will be non-null.

I've pushed patches for review, which should fix all occurrences so far.

Change 680192 merged by jenkins-bot:

[mediawiki/extensions/QuickSurveys@master] Also omit the edit count bucket when it's unavailable

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

Change 680180 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] Omit userEditCountBucket field for anons and when unavailable

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

I'm noticing a large number of errors related to this function in the the eventgate-validation dashboard.

'.event.editCountBucket' should be string, '.event.editCountBucket' should be equal to one of the allowed values

Logged to the DesktopWebUIActionsTracking schema.

Could that be related to the potential null return value?

Change 680800 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/WikimediaEvents@master] Send "0 edits" userEditCountBucket for anons

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

I'm noticing a large number of errors related to this function in the the eventgate-validation dashboard.
[...]
Could that be related to the potential null return value?

Yes, this is definitely the problem, thanks for flagging! It will be fixed by https://gerrit.wikimedia.org/r/680180 (merged) or also (slightly differently) https://gerrit.wikimedia.org/r/680800 (to be reviewed). I can deploy the hotfix early this week.

Change 681156 had a related patch set uploaded (by Krinkle; author: Awight):

[mediawiki/extensions/WikimediaEvents@wmf/1.37.0-wmf.2] Send "0 edits" userEditCountBucket for anons

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

Change 680800 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] Send "0 edits" userEditCountBucket for anons

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

Change 681156 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@wmf/1.37.0-wmf.2] Send "0 edits" userEditCountBucket for anons

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

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

[mediawiki/extensions/WikimediaEvents@wmf/1.37.0-wmf.1] Send "0 edits" userEditCountBucket for anons

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

I propose deploying the above as there are currently ~48 .event.editCountBucket-related validation errors/s and the -wmf.2 branch won't be deployed until next week.

I propose deploying the above as there are currently ~48 .event.editCountBucket-related validation errors/s and the -wmf.2 branch won't be deployed until next week.

Thanks for cherry-picking the backport and pointing out the severity! Deploying now...

Change 681334 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@wmf/1.37.0-wmf.1] Send "0 edits" userEditCountBucket for anons

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

Mentioned in SAL (#wikimedia-operations) [2021-04-21T11:29:09Z] <awight@deploy1002> Synchronized php-1.37.0-wmf.1/extensions/WikimediaEvents: Backport: [[gerrit:681334|Send 0 edits userEditCountBucket for anons (T210106)]] (duration: 00m 59s)

That looks good to me :)
I think with that we can mark this task as resolved (reopen if I'm mistaken).