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)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox48 --- fixed

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.
Blocks: 1220718
Whiteboard: [measurement:client]
Priority: -- → P3
Blocks: 1186986
No longer blocks: 1220718
Summary: Don't send empty histograms in Telemetry pings → Don't send empty keyed histograms in Telemetry pings
Assignee: nobody → allamsetty.anup
Mentor: alessio.placitelli
Depends on: 1172543
We should also add test coverage here.
Priority: P3 → P4
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)
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)
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
Assignee: nobody → mozilla
Mentor: gfritzsche
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
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).
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 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+
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)
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/
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 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+
Hi Claas, did you have a change to fix the small nits from comment 11?
Flags: needinfo?(mozilla)
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)
Attachment #8731444 - Attachment is obsolete: true
Attachment #8740616 - Attachment filename: 1247455_3.patch → 1231812_3.patch
Status: NEW → ASSIGNED
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+
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
https://hg.mozilla.org/mozilla-central/rev/a61539ca56bd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.