Closed Bug 1481669 Opened Last year Closed Last year

Fix DEVTOOLS_*_TIME_ACTIVE_SECONDS probes to report in seconds again

Categories

(DevTools :: General, defect, P1, major)

62 Branch
defect

Tracking

(firefox62+ verified, firefox63+ verified)

VERIFIED FIXED
Firefox 63
Tracking Status
firefox62 + verified
firefox63 + verified

People

(Reporter: Harald, Assigned: miker)

References

Details

Attachments

(2 files)

Broken since https://bugzilla.mozilla.org/show_bug.cgi?id=1458204#c13.

Tim and I discussed if the bug is a feature, but reporting the active time in milliseconds is not adding much value to the analysis, but leads to lost data as the last histogram bucket overflows.

This fix should be uplifted to beta.
Mike, do you have a ballpark number for how straightforward the fix would be?
Flags: needinfo?(mratcliffe)
Mike, any news. This impacts our tracking for impact of new features.
Adding flags for planning, we would like to get this fixed for 62 as well (and risk is minimal afaik).
We're building the final 62 beta today and the RC on Monday. This needs to be fixed ASAP if you want this in 62.
Flags: needinfo?(hkirschner)
Here is the place where TelemetryStopwatch counts the elapsed time:

https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/toolkit/components/telemetry/TelemetryStopwatch.jsm#380

It uses milliseconds and there is currently no way to customize it from DevTools code base.

-

Quick obvious fix we could land safely ASAP is to clone the entire TelemetryStopwatch.jsm
module and replace the code referenced above with something like as follows:

try {
  let delta = Cu.now() - startTime;
  return Math.round(delta / 1000);   // <- count in seconds
} catch (e) {
  ...
}

(we can't just simply introduce a prop like: `TelemetryStopwatch.useSec` since the module is shared)

And, we can follow up with proper fix...

(In reply to :Harald Kirschner :digitarald from comment #0)
> Tim and I discussed if the bug is a feature, but reporting the active time
> in milliseconds is not adding much value to the analysis, but leads to lost
> data as the last histogram bucket overflows.

I am not sure if I understand the 'overflow' issue, but it looks like there are a lot of other telemetry probes (all those based on TelemetryStopwatch.jsm) that are counting elapsed time in milliseconds. Why it wouldn't work for DevTools?


Honza
Sorry I missed the needinfo.

Honza's approach is probably the best we can do right now. Because of the structure of TelemetryStopwatch.jsm we can't even monkey patch it.
Flags: needinfo?(mratcliffe)
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Attached image Seconds.png
Screenshot of histograms in seconds
To test:
1. Open a few devtools tabs for a few seconds and then close the toolbox.
2. Go to about:telemetry
3. Search for `TIME_ACTIVE_SECONDS` under Histograms.
4. The histograms should be in seconds.
Comment on attachment 9003778 [details]
Bug 1481669 - Fix DEVTOOLS_*_TIME_ACTIVE_SECONDS probes to report in seconds again

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9003778 - Flags: review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d377aa2d4f98
Fix DEVTOOLS_*_TIME_ACTIVE_SECONDS probes to report in seconds again r=Honza
Thanks for working on this!

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> (In reply to :Harald Kirschner :digitarald from comment #0)
> > Tim and I discussed if the bug is a feature, but reporting the active time
> > in milliseconds is not adding much value to the analysis, but leads to lost
> > data as the last histogram bucket overflows.
> 
> I am not sure if I understand the 'overflow' issue, but it looks like there
> are a lot of other telemetry probes (all those based on
> TelemetryStopwatch.jsm) that are counting elapsed time in milliseconds. Why
> it wouldn't work for DevTools?

See e.g. the spike at the right of https://mzl.la/2wcYNwd -- histograms are defined with a maximum value, which cannot be changed later. Collecting in ms instead of s means the numerical values are 1000x greater than anticipated, so many observations exceed the maximum value and saturate. Milliseconds would have been fine if the original histogram definition had anticipated collecting values in milliseconds.
@Mike: don't forget request for uplift.

Thanks,
Honza
Flags: needinfo?(mratcliffe)
https://hg.mozilla.org/mozilla-central/rev/d377aa2d4f98
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment on attachment 9003778 [details]
Bug 1481669 - Fix DEVTOOLS_*_TIME_ACTIVE_SECONDS probes to report in seconds again

Approval Request Comment
[Feature/Bug causing the regression]:1458204
[User impact if declined]:We won't be able to properly see which users are using our devtools.
[Is this code covered by automated tests?]:Yes
[Has the fix been verified in Nightly?]:Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:None
[Is the change risky?]:No
[Why is the change risky/not risky?]:Very simple fix.
[String changes made/needed]:
Flags: needinfo?(mratcliffe)
Attachment #9003778 - Flags: approval-mozilla-beta?
Comment on attachment 9003778 [details]
Bug 1481669 - Fix DEVTOOLS_*_TIME_ACTIVE_SECONDS probes to report in seconds again

62 is on release now, moving the request over to there.
Attachment #9003778 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Comment on attachment 9003778 [details]
Bug 1481669 - Fix DEVTOOLS_*_TIME_ACTIVE_SECONDS probes to report in seconds again

Clearing the review flag here (review done in Phabricator)

Honza
Attachment #9003778 - Flags: review?(odvarko)
Comment on attachment 9003778 [details]
Bug 1481669 - Fix DEVTOOLS_*_TIME_ACTIVE_SECONDS probes to report in seconds again

Devtools telemetry fix. Approved for 62 RC1.
Attachment #9003778 - Flags: approval-mozilla-release? → approval-mozilla-release+
Flags: needinfo?(hkirschner) → qe-verify+
I could see the issue on Nightly 63.0a1 (20180807100107). This is fixed on Release 62.0 (20180827144429) and Nightly 63.0a1 (20180827100129) under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x32.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Follow up bug 1488363 reported (to avoid module cloning)
Honza
You need to log in before you can comment on or make changes to this bug.