Closed
Bug 1382820
Opened 6 years ago
Closed 6 years ago
Convert a11y_consumers and a11y_instantiated_flag to opt-out
Categories
(Toolkit :: Telemetry, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: jimm, Assigned: jimm)
References
(Blocks 2 open bugs)
Details
(Whiteboard: aes+)
Attachments
(1 file, 3 obsolete files)
3.87 KB,
patch
|
jimm
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
No description provided.
![]() |
Assignee | |
Comment 1•6 years ago
|
||
Attachment #8888518 -
Flags: review?(dbolter)
![]() |
Assignee | |
Comment 2•6 years ago
|
||
Comment on attachment 8888518 [details] [diff] [review] patch For data review. We'd like to collect accurate client counts out on release for these probes. The instantiated flag is a bool, no privacy issue there afaict. The consumers probe is an enum that detects injected dlls to identify specific clients, which is no different from the recent opt-out consumer scalar we landed.
Attachment #8888518 -
Flags: review?(benjamin)
Updated•6 years ago
|
Attachment #8888518 -
Flags: review?(dbolter) → review+
Updated•6 years ago
|
Priority: -- → P3
Comment 3•6 years ago
|
||
Comment on attachment 8888518 [details] [diff] [review] patch There are a few blocking issues and a bunch of nonblocking issues that should be followups: Blocking issues: * Both histograms need bug_numbers and alert_emails. alert_emails needs to have a real person who's responsible for the long-term value of the data (it may also have a list address). * The description of A11Y_CONSUMERS mentions an enum but not it's name, but I can't figure out what the recorded values mean. If there is an existing in-tree enumeration, at least point to it (by name or DXR link), or if there's not, the description should include what each value means. Non-blocking issues: * for A11Y_INSTANTIATED_FLAG, flag histograms are basically deprecated now, and it would be better to use a boolean scalar * both of these say record_in_processes main+content but I suspect that they are main-process-only and that would improve various aggregations
Attachment #8888518 -
Flags: review?(benjamin) → review-
Comment 4•6 years ago
|
||
Oh, and when you add bug_numbers and alert_emails you can remove these histograms from the whitelist of old histograms that don't follow the rules (histogram-whitelists.json)
Comment 5•6 years ago
|
||
I forgot one non-blocking issue (can be a followup): typically opt-out metrics need to have a unit test to ensure that they still work. This is important because code refactorings fairly frequently break telemetry without people noticing. So I would strongly encourage you to try and get basic unit tests working for these metrics. I realize this might be difficult because our testing framework probably doesn't do much a11y.
![]() |
Assignee | |
Updated•6 years ago
|
Whiteboard: aes+
![]() |
Assignee | |
Comment 6•6 years ago
|
||
- added bug#s (this bug) to the two probes I'm converting to opt-out - added emails for all four probes - removed entries from white lists - filed two follow ups, converting to boolean scalars and adding unit tests
Attachment #8888518 -
Attachment is obsolete: true
Attachment #8889893 -
Flags: review?(benjamin)
Comment 7•6 years ago
|
||
Comment on attachment 8889893 [details] [diff] [review] patch data-r=me thanks!
Attachment #8889893 -
Flags: review?(benjamin) → review+
![]() |
Assignee | |
Comment 8•6 years ago
|
||
- updating email address to something more generic
Attachment #8889893 -
Attachment is obsolete: true
Attachment #8889894 -
Flags: review?(benjamin)
![]() |
Assignee | |
Comment 9•6 years ago
|
||
Comment on attachment 8889894 [details] [diff] [review] patch + from irc
Attachment #8889894 -
Flags: review?(benjamin) → review+
![]() |
Assignee | |
Comment 10•6 years ago
|
||
s/been used/has been used text in comments.
Attachment #8889894 -
Attachment is obsolete: true
Attachment #8889896 -
Flags: review+
![]() |
Assignee | |
Updated•6 years ago
|
Keywords: checkin-needed
Comment 11•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/547d9dfbbf67 Convert a11y_consumers and a11y_instantiated_flag to opt-out. r=davidb, r=bsmedberg
Keywords: checkin-needed
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/547d9dfbbf67
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
![]() |
Assignee | |
Comment 13•6 years ago
|
||
Comment on attachment 8889896 [details] [diff] [review] patch Approval Request Comment [Feature/Bug causing the regression]: converting existing a11y telemetry probes to opt out [User impact if declined]: we'd like tyo have this data out on release for 55 so we can make some decisions about 56/57 rollout. [Is this code covered by automated tests?]: telemetry is [Has the fix been verified in Nightly?]: changed probe to opt out [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?]: existing well test probe converted to opt out on release [String changes made/needed]: none
Attachment #8889896 -
Flags: approval-mozilla-beta?
Comment 14•6 years ago
|
||
Comment on attachment 8889896 [details] [diff] [review] patch convert 2 a11y telemetry probes to opt-out, needed in 55 release. should be in 55.0b13
Attachment #8889896 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 15•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e2dd499956f0
status-firefox55:
--- → fixed
Comment 16•6 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #13) > [Is this code covered by automated tests?]: > telemetry is > [Has the fix been verified in Nightly?]: > changed probe to opt out > [Needs manual test from QE? If yes, steps to reproduce]: > no Setting qe-verify- based on Jim's assessment on manual testing needs.
Flags: qe-verify-
![]() |
Assignee | |
Updated•6 years ago
|
Target Milestone: mozilla56 → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•