Closed Bug 1529696 Opened 8 months ago Closed 7 months ago

Rename nsITelemetry's datasets (DATASET_RELEASE_CHANNEL_{OPTIN|OPTOUT}) to something less obtuse

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement
Points:
1

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: chutten, Assigned: janerik)

References

Details

Attachments

(2 files, 2 obsolete files)

We use nsITelemetry's "dataset" values to determine what subset of recorded data to return from our snapshotting functions. We also use them to identify which set of recorded data a measurement belongs to.

For instance, GC_MS is only recorded in "nightly" and "beta" channels. This means it is a DATASET_RELEASE_CHANNEL_OPTIN measure and that a snapshot of DATASET_RELEASE_CHANNEL_OPTIN should include it if it has samples. (and that a snapshot of DATASET_RELEASE_CHANNEL_OPTOUT should not include it, even if it has samples)

I think we all agree that the existing names are frustrating to understand and use. Let's finally rename them to something better.

For the record, DATASET_RELEASE_CHANNEL_OPTIN means "Give me all of the data" and ..._OPTOUT means "Give me only the base subset of data we'd report on non-prerelease channels".

In bug 1527299 Georg attached a patch that renames:

  • DATASET_RELEASE_CHANNEL_OPTIN to DATASET_PRERELEASE_CHANNELS
  • DATASET_RELEASE_CHANNEL_OPTOUT to DATASET_ALL_CHANNELS

Does anyone have a better naming scheme than the one Georg proposes? Does anyone have a reason we shouldn't rename these things at all?

Notes:

  • The Probe dictionary uses the naming scheme "population" with values "prerelease" and release.
  • Definitions files (Histograms.json, Scalars.yaml, Events.yaml) use something like "releaseChannelCollection": "opt-out" to identify Histograms that are part of ..._OPTOUT
  • We've in the past called these "base" collection for things recorded everywhere and "extended" collection for things only recorded in prerelease.
  • This isn't going to change the wording of the definitions files, just the constants on nsITelemetry. If we can come up with a naming system we can use consistently in both places, I'd call that a bonus.
Flags: needinfo?(tlong)
Flags: needinfo?(jrediger)
Flags: needinfo?(alessio.placitelli)

I like the connotations of "base" and "extended" personally. I think they are the most descriptive adjectives from what I understand of the differences between the two sets.

Georg's naming scheme is also descriptive but I must point out that DATASET_PRERELEASE_CHANNELS and DATASET_ALL_CHANNELS gives me information about where they should be used, while "base" and "extended" impart information about the contents.

When I searched for the places where these are used in code, based on the context, I think that "base" and "extended" make the most sense from a naming standpoint since I'm less likely to wonder where I should use this constant rather than wonder what set of information should be conveyed with it.

I definitely defer to those who have more experience with the codebase and history, but, from an outside viewpoint, there ya go!

Flags: needinfo?(tlong)

(In reply to Chris H-C :chutten from comment #0)

For the record, DATASET_RELEASE_CHANNEL_OPTIN means "Give me all of the data" and ..._OPTOUT means "Give me only the base subset of data we'd report on non-prerelease channels".

In bug 1527299 Georg attached a patch that renames:

  • DATASET_RELEASE_CHANNEL_OPTIN to DATASET_PRERELEASE_CHANNELS
  • DATASET_RELEASE_CHANNEL_OPTOUT to DATASET_ALL_CHANNELS

Does anyone have a better naming scheme than the one Georg proposes? Does anyone have a reason we shouldn't rename these things at all?

I feel like we should push/stick to the "prerelease" vs "release" notion, because it resonates with how things work currently in our telemetry world, how we turn things on or off by default, etc. Moreover, we're trying our best to extend the same concept to the mobile world, so it would help being consistent.

I'm not sure I have better ideas: I liked the "base" vs "extended" combo before, but it feels like doesn't really suit our prerelease vs releease world anymore.

Flags: needinfo?(alessio.placitelli)

(In reply to Alessio Placitelli [:Dexter] from comment #2)

I feel like we should push/stick to the "prerelease" vs "release" notion, [...]

The problem with "prerelease" vs "release" is: "optin" is "prerelease only" and "optout" is actually "release + prerelease" (and not just release).

Another point worth mentioning:
The new snapshot APIs (getSnapshotFor*) got rid of the dataset argument, making it unnecessary for anyone to actually pass DATASET_RELEASE_CHANNEL_{OPTIN,OPTOUT}.
The only way to change between datasets in tests is now toggling Telemetry.canRecordExtended before calling the snapshot function.
The only remaining function that takes this argument is snapshotEvents.

For that case I think I'd be ok with having DATASET_PRERELEASE_CHANNELS and DATASET_ALL_CHANNELS.

One downside obviously of renaming the constants is the mismatch with the definition file (and it's much more likely that people have to write/read the definition file than use the DATASET_* constant)

Flags: needinfo?(jrediger)

(In reply to Jan-Erik Rediger [:janerik] from comment #3)

For that case I think I'd be ok with having DATASET_PRERELEASE_CHANNELS and DATASET_ALL_CHANNELS.

Much clearer now, thanks. This new naming seems better suited then!

Attachment #9045966 - Attachment is obsolete: true
Assignee: chutten → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2

Could you fit this into the next iteration?

Flags: needinfo?(jrediger)
Assignee: nobody → jrediger
Flags: needinfo?(jrediger)
Priority: P2 → P1
Attachment #9045967 - Attachment is obsolete: true
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/881371ffa966
Rename DATASET_RELEASE_CHANNEL_OPTOUT/OPTIN to DATASET_ALL/PRERELEASE_CHANNELS in Telemetry code r=chutten
https://hg.mozilla.org/integration/autoland/rev/bd786eb15b54
Rename DATASET_RELEASE_CHANNEL_OPTOUT/OPTIN to DATASET_ALL/PRERELEASE_CHANNELS everywhere r=chutten
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Blocks: 1542842
Regressions: 1553181
You need to log in before you can comment on or make changes to this bug.