Rename nsITelemetry's datasets (DATASET_RELEASE_CHANNEL_{OPTIN|OPTOUT}) to something less obtuse
Categories
(Toolkit :: Telemetry, enhancement, P1)
Tracking
()
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_OPTINtoDATASET_PRERELEASE_CHANNELSDATASET_RELEASE_CHANNEL_OPTOUTtoDATASET_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.
|
||
Comment 1•5 years ago
|
||
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!
|
||
Comment 2•5 years ago
|
||
(In reply to Chris H-C :chutten from comment #0)
For the record,
DATASET_RELEASE_CHANNEL_OPTINmeans "Give me all of the data" and..._OPTOUTmeans "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_OPTINtoDATASET_PRERELEASE_CHANNELSDATASET_RELEASE_CHANNEL_OPTOUTtoDATASET_ALL_CHANNELSDoes 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.
|
Assignee | |
Comment 3•5 years ago
|
||
(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)
|
|
||
Comment 4•5 years ago
|
||
(In reply to Jan-Erik Rediger [:janerik] from comment #3)
For that case I think I'd be ok with having
DATASET_PRERELEASE_CHANNELSandDATASET_ALL_CHANNELS.
Much clearer now, thanks. This new naming seems better suited then!
|
Reporter | |
Comment 5•5 years ago
|
||
|
||
Updated•5 years ago
|
|
|
Reporter | |
Comment 6•5 years ago
|
||
|
|
Reporter | |
Updated•5 years ago
|
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 8•5 years ago
|
||
|
|
Assignee | |
Comment 9•5 years ago
|
||
Depends on D25934
|
|
||
Updated•5 years ago
|
|
||
Comment 10•5 years ago
|
||
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
|
||
Comment 11•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/881371ffa966
https://hg.mozilla.org/mozilla-central/rev/bd786eb15b54
|
||
Updated•5 years ago
|
Description
•