Don't send empty keyed histograms in Telemetry pings

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
Telemetry
P4
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: claas, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox48 fixed)

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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

2 years ago
Blocks: 1220718
Whiteboard: [measurement:client]
(Reporter)

Updated

2 years ago
Priority: -- → P3
(Reporter)

Updated

2 years ago
Blocks: 1186986
No longer blocks: 1220718
Summary: Don't send empty histograms in Telemetry pings → Don't send empty keyed histograms in Telemetry pings
(Reporter)

Updated

2 years ago
Assignee: nobody → allamsetty.anup
(Reporter)

Updated

2 years ago
Mentor: alessio.placitelli@gmail.com
(Reporter)

Updated

2 years ago
Depends on: 1172543
(Reporter)

Comment 1

2 years ago
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)
(Reporter)

Comment 3

2 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)
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
Assignee: nobody → mozilla
Mentor: gfritzsche@mozilla.com
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).
(Assignee)

Comment 6

2 years ago
Created attachment 8728139 [details] [diff] [review]
TelemetrySession.jsm (skip empty keyed histograms) + test_TelemetrySession.js (changed test expectations)

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/
(Assignee)

Comment 10

2 years ago
Created attachment 8731444 [details] [diff] [review]
TelemetrySession.jsm (skip empty keyed histograms) + test_TelemetrySession.js (changed test expectations) + main-ping.rst (updated documentation)

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)
(Assignee)

Comment 13

2 years ago
Created attachment 8740616 [details] [diff] [review]
TelemetrySession.jsm (skip empty keyed histograms) + test_TelemetrySession.js (changed test expectations) + main-ping.rst (updated documentation)

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

2 years ago
Attachment #8731444 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8740616 - Attachment filename: 1247455_3.patch → 1231812_3.patch
(Assignee)

Updated

2 years ago
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

Comment 16

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/a61539ca56bd
Keywords: checkin-needed

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a61539ca56bd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.