Fix probe recording range display #543
Conversation
|
Thanks! Looking at DEVTOOLS_TOOLBOX_TIME_ACTIVE_SECONDS, I wonder if reporting it as "release from 30" is the right presentation; it became an opt-out metric in release for 60. |
|
(The original issue with the _COUNT probe looks good now!) |
|
Good catch! I addressed that, but now DEVTOOLS_TOOLBOX_TIME_ACTIVE_SECONDS is showing up as recorded from 59 on release. This seems to actually come from the source data, i.e. probe-scraper. |
|
I found another example where recording ranges are off, telemetry.discarded.child_events. Per 1369041 this probe should be on 56+ and not be on 55 for release & beta. |
|
@tdsmith Can you take a look, with that issue in mind? |
|
Hmm, sorry to nitpick, but the table view in the staging version lists expired prerelease probes as "recorded: never" now. Compare the "recorded" columns: |
|
DOM_TIMERS_* are another interesting set of probes; they're currently displayed as recorded from "46 to 39" on t.m.o |
|
Good catches, cheers! @tdsmith Do you want to take another look? |
|
Looking great! |
There was a problem hiding this comment.
This looks good to me! Consider adding the comments where requested below, but don't block on this if you don't have time right now. It's something that would make it easier for other readers in the future, but doesn't need to happen in this bug, as it's a project-wide effort!
probe-dictionary/explore.js
Outdated
| function friendlyRecordingRange(firstVersion, expiry) { | ||
| if (expiry == "never") { | ||
| return `from ${firstVersion}`; | ||
| function friendlyRecordingRangeForHistory(history, channel, filterPrerelease) { |
There was a problem hiding this comment.
This reads much better now! I wish this was just a little more documented, as it's still difficult to parse after not looking at this for a while.
There was a problem hiding this comment.
That's a good point, i will add some.
probe-dictionary/explore.js
Outdated
| function friendlyRecordingRangeForHistory(history, channel, filterPrerelease) { | ||
| const last = array => array[array.length - 1]; | ||
|
|
||
| if ((channel == "release") && filterPrerelease) { |
There was a problem hiding this comment.
comment nit: why is this fixup needed? That's for bridging the optout/optin to the prerelease/release definitions, right?
There was a problem hiding this comment.
On the "release" channel we only want to account for the release probes. The most readable approach seems to just filter out the history parts we don't want to account for.
This is only used for the detail view though; in the search results view we want different behavior - hence the filterPrerelease flag.
probe-dictionary/explore.js
Outdated
| const expiry = state.expiry_version; | ||
| return friendlyRecordingRange(firstVersion, expiry); | ||
| } | ||
| if (history.length == 0) { |
There was a problem hiding this comment.
Why are we returning "never" if there's no history? Shouldn't this just be the case for expiry == "never"?
There was a problem hiding this comment.
The prior filter statement can result in an empty history (consider prerelease-only probes on "release" channel).
That should result in a "never" recording range on that channel.
| @@ -461,7 +471,7 @@ function renderMeasurements(measurements) { | |||
| ["name", (d, h, c) => d.name], | |||
| ["type", (d, h, c) => d.type], | |||
| ["population", (d, h, c) => h.optout ? "release" : "prerelease"], | |||
| ["recorded", (d, h, c, history) => friendlyRecordingRangeForHistory(history, c)], | |||
| ["recorded", (d, h, c, history) => friendlyRecordingRangeForHistory(history, c, false)], | |||
There was a problem hiding this comment.
nit: should we add /* filterPrelrease */ after false? It would make it easier to understand without having tor scroll the file back to the function definition
There was a problem hiding this comment.
That makes these lines pretty long though, i'm torn on that.
There was a problem hiding this comment.
Feel free to drop this and the one below!
| @@ -843,7 +853,7 @@ function showDetailViewForId(probeId, channel=$("#select_channel").val()) { | |||
| if ((!history[0].optout && (ch == "release")) || (ch == "aurora")) { | |||
| continue; | |||
| } | |||
| rangeText.push(`${ch} ${friendlyRecordingRangeForHistory(history, ch)}`); | |||
| rangeText.push(`${ch} ${friendlyRecordingRangeForHistory(history, ch, true)}`); | |||
There was a problem hiding this comment.
nit: same here, but only if it works in the ` :)
tdsmith found an issue with the shown recording in the probe-dictionary.
Investigating this it turned out to be a problem with the frontend.
The changes are up for testing here.