Toolbox open event telemetry is no longer firing because of this.win.top usage
Categories
(DevTools :: Framework, defect, P2)
Tracking
(firefox-esr60 unaffected, firefox-esr68 wontfix, firefox68 wontfix, firefox69 fixed, firefox70 fixed)
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | wontfix |
firefox68 | --- | wontfix |
firefox69 | --- | fixed |
firefox70 | --- | fixed |
People
(Reporter: Harald, Assigned: jdescottes)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(2 files)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
6.69 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
Analysis per :tdsmith from bug 1571483.
https://hg.mozilla.org/mozilla-central/rev/20f11a17eace01fdcf67352222a458b3b493ce0d
it looks like the pending event is still associated with this.win.top, and the properties are now being collected on this.win, so the pending event never fires?
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Backed out changeset 3f0cde4a61e5 (bug 1572867) for causing browser_toolbox_telemetry_open_event.js to perma fail
backout: https://hg.mozilla.org/integration/autoland/rev/494e151853d11572d4b8276aa27bdde4c822ed31
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
I am not sure this is a permafail, I see the test running fine in the test-verify jobs. I guess there is a race, maybe the telemetry event is registered only after toolbox-ready in some cases.
Assignee | ||
Comment 5•5 years ago
|
||
Ah it might be because it's running in isolation in test-verify, maybe the event is not fired when running after another test.
Comment 7•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•5 years ago
|
Comment 8•5 years ago
|
||
I guess this was regressed in the timeframe of https://bugzilla.mozilla.org/show_bug.cgi?id=1539979#c19. FWIW, this is why we generally try to avoid having work land across that long of a period of time - it really makes tracking a pain when changes are landing across a 4 month span in a single bug.
Please nominate this for Beta and ESR68 approval, I guess...
Assignee | ||
Comment 9•5 years ago
|
||
Sorry about that, I usually avoid using leave-open bugs for the same reason.
Assignee | ||
Comment 10•5 years ago
|
||
Comment on attachment 9085177 [details]
Bug 1572867 - Use toolbox topWindow to prepare telemetry "open" event
Beta/Release Uplift Approval Request
- User impact if declined: No user impact, but we will miss part of the telemetry data we record for DevTools
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Small javascript fix, covered by automated tests
- String changes made/needed:
Assignee | ||
Comment 11•5 years ago
|
||
(needs a different version of the patch for ESR68)
Assignee | ||
Comment 12•5 years ago
|
||
Harald, do you think this is worth asking for a 68 ESR uplift? I am not sure how critical this data is for us on ESR?
Comment 13•5 years ago
|
||
Comment on attachment 9085177 [details]
Bug 1572867 - Use toolbox topWindow to prepare telemetry "open" event
Fixes broken Telemetry collection for Devtools in some cases. Approved for 69.0b16.
Comment 14•5 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 15•5 years ago
|
||
ESR should not be needed (not sure how high this bar is to uplift there, but not having this telemetry has not hurt our tracking so far as other telemetry can fill it in).
Updated•5 years ago
|
Updated•3 years ago
|
Description
•