Closed
Bug 1231812
Opened 8 years ago
Closed 8 years ago
Don't send empty keyed histograms in Telemetry pings
Categories
(Toolkit :: Telemetry, defect, P4)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: Dexter, Assigned: claas, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [measurement:client] [lang=js])
Attachments
(1 file, 2 obsolete files)
We are submitting empty key histograms with our pings (base/extended): they show up as "DEVTOOLS_WEBIDE_CONNECTED_RUNTIME_VERSION" : {}, and don't really add any useful information to the ping. But I think they do have an impact on storage costs.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P3
Reporter | ||
Updated•8 years ago
|
Updated•8 years ago
|
Summary: Don't send empty histograms in Telemetry pings → Don't send empty keyed histograms in Telemetry pings
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → allamsetty.anup
Reporter | ||
Updated•8 years ago
|
Mentor: alessio.placitelli
Reporter | ||
Comment 1•8 years ago
|
||
We should also add test coverage here.
Updated•8 years ago
|
Priority: P3 → P4
Comment 2•8 years ago
|
||
I might be a bit slow these days because of my internship. If you feel that this needs to be done soon, I would be happy to leave this bug, and will try to catch up with the rest of the work regarding the "telemetry pings".
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
Reporter | ||
Comment 3•8 years ago
|
||
This doesn't need to be done soon, as it's not an high priority item for this quarter. Let me unassign it, you can always come back to it if/when you have more time!
Assignee: allamsetty.anup → nobody
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
Updated•8 years ago
|
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
Updated•8 years ago
|
Assignee: nobody → mozilla
Updated•8 years ago
|
Mentor: gfritzsche
Comment 4•8 years ago
|
||
The payload for "main" and "saved-session" pings is assembled in this module: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm We can check that this is working properly in about:telemetry. We would want to update this file with test coverage (more details on that later): https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
Comment 5•8 years ago
|
||
Following up here... Once this is working fine and confirmed using about:telemetry, we could fix the tests. To run the test: mach test toolkit/components/telemetry/tests/unit/test_TelemetrySession.js To run all telemetry unit tests: mach test toolkit/components/telemetry/tests/unit/ We will definitely have to adjust test_TelemetrySession.js around here a bit: https://dxr.mozilla.org/mozilla-central/rev/2b5237c178ea/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js#648 We will have to assert that the keyed test histograms are not in the payload when they are empty (and assert them being present and having specific values etc. once we add()ed something to them).
Assignee | ||
Comment 6•8 years ago
|
||
I updated TelemetrySession.jsm to skip empty keyed histograms in Impl.getKeyedHistograms() and - after checking that it is working properly in about:telemetry - I also updated the test expectations in test_TelemetrySession.js.
Flags: needinfo?(gfritzsche)
Comment 7•8 years ago
|
||
Comment on attachment 8728139 [details] [diff] [review] TelemetrySession.jsm (skip empty keyed histograms) + test_TelemetrySession.js (changed test expectations) Review of attachment 8728139 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks! I only have a minor request below. When uploading the updated patch, you can set the "review" flag to "?" and choose me as the reviewer. ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +938,5 @@ > snapshot = keyed.snapshot(); > } > + > + let keys = Object.keys(snapshot); > + if (keys.length > 0) { We should add a comment here that we intentionally skip empty keyed histograms. Nit: I think the "if (empty) { continue }" pattern would be more readable here (less nesting).
Attachment #8728139 -
Flags: feedback+
Comment 8•8 years ago
|
||
I nearly forgot, we should also update the documentation here and mention that empty histograms are not submitted: https://dxr.mozilla.org/mozilla-central/rev/4657041c6f77b36b1fb9647c28f53f4f51757360/toolkit/components/telemetry/docs/main-ping.rst#181 If you want to check how the documentation looks, it's generated using "mach docs". The command will show you in which directory you'll find the main "index.html" after it finished.
Flags: needinfo?(gfritzsche)
Comment 9•8 years ago
|
||
We should also add a note to the documentation that before Firefox 48 (that's what's we are currently developing on per [0]), empty keyed histograms were submitted as {}. 0: http://whattrainisitnow.com/
Assignee | ||
Comment 10•8 years ago
|
||
The patch contains the changes from the previous patch plus the following changes: - use continue to skip empty keyed histograms (less nesting) - add comment to document the intention of the skip - update ping documentation wrt. keyed histograms
Attachment #8728139 -
Attachment is obsolete: true
Attachment #8731444 -
Flags: review?(gfritzsche)
Comment 11•8 years ago
|
||
Comment on attachment 8731444 [details] [diff] [review] TelemetrySession.jsm (skip empty keyed histograms) + test_TelemetrySession.js (changed test expectations) + main-ping.rst (updated documentation) Review of attachment 8731444 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good to me :) Only two minor nits below, also let's make it "r=gfritzsche" in the commit message. If you can fix those and upload the updated patch, then we can land this. ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +938,5 @@ > snapshot = keyed.snapshot(); > } > + > + let keys = Object.keys(snapshot); > + if (keys.length === 0) { Nit: |... == 0| is enough. @@ +939,5 @@ > } > + > + let keys = Object.keys(snapshot); > + if (keys.length === 0) { > + // skip empty keyed histogram Nit: Lets start comments with an upper-case letter and finish with a '.'.
Attachment #8731444 -
Flags: review?(gfritzsche) → review+
Comment 12•8 years ago
|
||
Hi Claas, did you have a change to fix the small nits from comment 11?
Flags: needinfo?(mozilla)
Assignee | ||
Comment 13•8 years ago
|
||
The patch contains the changes from the previous patch plus the following changes: - Change comparison to '=='. - Update comment to start with upper-case letter and finish with a '.'. (- Solve merge conflict.)
Flags: needinfo?(mozilla)
Attachment #8740616 -
Flags: review?(gfritzsche)
Assignee | ||
Updated•8 years ago
|
Attachment #8731444 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8740616 -
Attachment filename: 1247455_3.patch → 1231812_3.patch
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 14•8 years ago
|
||
Comment on attachment 8740616 [details] [diff] [review] TelemetrySession.jsm (skip empty keyed histograms) + test_TelemetrySession.js (changed test expectations) + main-ping.rst (updated documentation) Review of attachment 8740616 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks!
Attachment #8740616 -
Flags: review?(gfritzsche) → review+
Comment 15•8 years ago
|
||
No need to push this to try: Tests ran through fine locally, SEARCH_COUNTS look fine and it's a change with limited scope.
Keywords: checkin-needed
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a61539ca56bd
Keywords: checkin-needed
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a61539ca56bd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•