@Sfaci This is a use case for the refactoring work you're doing now. Please keep it in mind in that work.

@Sfaci This is a use case for the refactoring work you're doing now. Please keep it in mind in that work.

Ok! It probably makes sense to work on both things at the same time

@mpopov And now that I'm reading this task, I have something I would like to comment about the logExposure method and the set of contextual attributes we want to include with the generated event:

• I'm afraid that the current JS implementation might not allow adding additional contextual attributes. Contextual attributes are fetched in the last step before calling the submit method inside MetricsClient. I mean, logExposure would call Experiment::send that calls MetricsClient::submitInteracion that fetches contextual attributes based only in the defined stream and calls MetricsClient::submit . Not sure yet if there is a good way to put somewhere those additional contextual attributes. Can we implement like a custom send+submitInteraction+submit method to try to deal with that? Probably yes but I don't know if it's something we want to do (should we refactor first the JS implementation to be more according to the current PHP one?)
• The current PHP implementation is more flexible and it's very easy to add new contextual attributes before calling send and all of them will be considered when submitting the event. That works right now

And the question is, would it make sense/would it be useful to implement for now the logExposure method for both JS and PHP implementation having only the latter those contextual attributes included in the generated experiment_exposure event? Refactoring the JS implementation is in our TODO list and, if we follow same approach as the PHP one, it would be easy to deal with additional contextual attributes before sending a event via send method

Asking in advance, I'm still trying to figure out a different approach to be able to add those exposure contextual attributes for the JS case

Change #1239018 had a related patch set uploaded (by Santiago Faci; author: Santiago Faci):

[mediawiki/extensions/TestKitchen@master] JS and PHP SDK: Add logExposure method

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

Isn't the problem above related to the JS SDK part we would have to implement in T408186: Configure experiments with stream, schema and contextual attributes?

The way MetricsPlatform instantiates an experiment has been redefined for experiments to be able to use their own configuration (stream, schema, contextual attributes) when sending events

In the PHP SDK contextual_attributes is part of the experimentConfig already so we can add additional ones but we don't have that thing for the JS one (probably because we are still using MetricsClient?)

@Sfaci: Sorry for last minute scope change, I just realized we also need performer_is_bot as one of the contextual attributes.

In T414726#11611197, @mpopov wrote:

@Sfaci: Sorry for last minute scope change, I just realized we also need performer_is_bot as one of the contextual attributes.

No worries!
Noted! I'm still refining the corresponding patch for the PHP side

Note that the current patch only includes the curated set of contextual attributes for the PHP SDK. The JS one is for now blocked by T415579: Migrate JS and PHP client libraries to TestKitchen extension

@Sfaci @KReid-WMF: @phuedx and I were talking about the future where metric specifications specify which contextual attributes should be collected per event, rather than per instrument/experiment. We've seen a need for this already, both with the exposure logging work and Editing & Growth's experiments where only edit_saved events need page_namespasce_id and page_revision_id but those aren't needed for any other events.

What do you think about solving this task with a generalized solution, rather than a specific one? What if we had a version of send() that accepted an array of contextualAttributes?

Then logExposure() becomes a wrapper to

send( 'experiment_exposure', contextualAttributes = [
  'mediawiki_database',
  'performer_is_logged_in',
  'performer_is_temp',
  'performer_is_bot',
] );

and instead of

send( 'edit_saved', {
	page: {
		namespace_id: mw.config.get( 'wgNamespaceNumber' ),
		revision_id: data.newrevid
	}
} );

Editing's instrument would just have send( 'edit_saved', contextualAttributes = [ 'page_namespace_id', 'page_revision_id' ] )

(I don't know if or how named arguments work in JS land, so this is pseudocode.)

send() would then be responsible for merging the per-event contextual attributes with the per-data-collection-activity contextual attributes (from instrument/experiment config).

send() would then be responsible for merging the per-event contextual attributes with the per-data-collection-activity contextual attributes (from instrument/experiment config).

So I assume that these per-event contextual attributes won't be defined in the stream config. A developer will be able to add contextual attributes as per-event ones freely

What do you think about solving this task with a generalized solution, rather than a specific one? What if we had a version of send() that accepted an array of contextualAttributes?

Sounds good!. send could just send the contextual attributes defined in the stream config when contextualAttributes isn't passed as parameter and could add also the ones defined in that parameter when passed. And I would even say there would be ways to avoid considering this like a breaking change which is always a good thing

The only blocker right now for this would be the way JS SDK works (because the contextual attributes are taken exclusively from the stream config) but we are already working on that via T415579: Migrate JS and PHP client libraries to TestKitchen extension and I guess the approach will be the one was done for the PHP SDK, where we can add contextual attributes to the ones defined in the stream config to send them all together.

So I would say that we can plan this work for when we finish to migrate JS SDK to TestKitchen extension. At that point we will be able to implement this feature for both JS and PHP SDK

In T414726#11615881, @Sfaci wrote:

The only blocker right now for this would be the way JS SDK works (because the contextual attributes are taken exclusively from the stream config) but we are already working on that via T415579: Migrate JS and PHP client libraries to TestKitchen extension and I guess the approach will be the one was done for the PHP SDK, where we can add contextual attributes to the ones defined in the stream config to send them all together.

To confirm, the patch I submitted for T415579 has per-event contextual attributes in mind (but doesn't go as far as adding a contextualAttributes parameter to ::send()).

Change #1239018 merged by jenkins-bot:

[mediawiki/extensions/TestKitchen@master] JS and PHP SDK: Add sendExposure method

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

The patch above, which is already merged, already includes, thanks to some Sam's feedback, a send($action, [$interactionData], [$contextualAttributes]) method for the PHP SDK.

T415579: Migrate JS and PHP client libraries to TestKitchen extension will be merged soon and that will allow us to work on adding the curated set of contextual attributes for the sendExposure function in the JS SDK. We'll take also the opportunity to refactor send to have that third optional parameter to pass the per-event contextual attributes. Then, both PHP and JS SDKs will be fully aligned regarding exposure events and the per-event contextual attributes feature.

Change #1242524 had a related patch set uploaded (by Santiago Faci; author: Santiago Faci):

[mediawiki/extensions/TestKitchen@master] JS SDK: curated set of contextual attributes with exposure events

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

Change #1242524 merged by jenkins-bot:

[mediawiki/extensions/TestKitchen@master] JS SDK: curated set of contextual attributes with exposure events

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