Use an experiment, rather than a flag, for the last scan date

[Vinnl]

References:

Jira: MNTOR-3094

Description

This builds on #4407, but replaces the feature flag by an experiment.

It incorporates changes from #4438 to determine the experimentationId (but to limit the number of changes in this PR, I didn't rename the function yet from getUserId). However, in contrast to that PR, it doesn't repurpose our feature flag code, but avoids it altogether. The idea is that we'd fully remove our feature flag infrastructure in favour of experiments - and to see the experiments locally, you'd set the default value in nimbus.yml. That makes sure we have a single source of truth, i.e. the experiment, without losing strict typing.

How to test

Enable the last-scan-date experiment in nimbus.yml via its default value. The last scan date should be displayed like it used to be with the flag.

Checklist (Definition of Done)

• Localization strings (if needed) have been added.
• Commits in this PR are minimal and have descriptive commit messages.
• I've added or updated the relevant sections in readme and/or code comments
• I've added a unit test to test for potential regressions of this bug.
• Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
• All acceptance criteria are met.
• Jira ticket has been updated (if needed) to match changes made during the development process.
• Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

[rhelmer]

This approach lgtm, if we go this way it means that we are going all-in on Nimbus experiments and not doing a gradual transition.

So new features need to decide if they are using feature flags (which will be deprecated, I could do make that explicit in #4438) or if they are using Nimbus experiment. There's some overhead to set up a roll-out or experiment, but maybe you're right and it'd be best to go all-in and not try to combine these systems.

One question I have is how do we test this locally? Could be have a local environment in nimbus.yaml which would let us set the defaults maybe?

[mansaj]

LGTM! Very cool

[rhelmer]

Sorry I just double-checked and noticed that you didn't pull over the https://dictionary.telemetry.mozilla.org/apps/mdn_yari/metrics/glean_client_annotation_experimentation_id change - we need to switch to using the experimentationId argument to Glean.initialize().

Right now the PageLoadEvent sets it as user_id, but we don't want it in the payload and we need it to be set for all events not just this one.

I reverted it from my branch but you can still find the code I used in 779f6e2 (e.g.

blurts-server/src/app/hooks/useGlean.ts

Line 45 in 779f6e2

experimentationId,

) or I can do this in a separate PR if you like.

[Vinnl]

Thanks @rhelmer, good catch. The commit you linked was empty, but I think the right one is 4705929#diff-a90f1e99ca381caeb8fe87186b6c8aa0afeb082d4d108c3231710d23d2238c0f? I incorporated those changes in 1519b6b.

I'm not sure what you mean by

Right now the PageLoadEvent sets it as user_id, but we don't want it in the payload and we need it to be set for all events not just this one.

Do you mean updating every call to useTelemetry to set it?

I also went ahead and renamed userId to experimentationId (73f52e5). When doing so, I noticed that you'd also removed it from page.view events (replicated in 2d60e66) - should we also remove that from metrics.yaml?

[rhelmer]

Thanks @rhelmer, good catch. The commit you linked was empty, but I think the right one is 4705929#diff-a90f1e99ca381caeb8fe87186b6c8aa0afeb082d4d108c3231710d23d2238c0f? I incorporated those changes in 1519b6b.

I'm not sure what you mean by

Right now the PageLoadEvent sets it as user_id, but we don't want it in the payload and we need it to be set for all events not just this one.

Do you mean updating every call to useTelemetry to set it?

I also went ahead and renamed userId to experimentationId (73f52e5). When doing so, I noticed that you'd also removed it from page.view events (replicated in 2d60e66) - should we also remove that from metrics.yaml?

I've been discussing with DS what to do with this, I think we're going to use user_id as a Monitor-specific identifier since we're not going to be able to share the Mozilla Accounts ID right now (which is what the description says it does and isn't true). So at minimum I'm going to change the description and make sure we're OK to collect a Monitor-specific user identifier here.

The experimentationId is explicitly separate from user_id or fxa_uid and intended only for experimentation, thanks for making that change!

[flodolo]

Approving to get it out of the queue, but I feel like I've reviewed the same couple of strings a few time over the last week?

[Vinnl]

Sorry @flodolo, that happens when I base a PR on a different PR. As the original PR gets rebased and then merged into main, this PR gets retargeted on main, and then the commits that used to be in the orignal PR will be seen as part of this one - including changes to .ftl files. And because you're listed in CODEOWNERS, GitHub will then automatically request the review from you (even though it says that I requested it myself).