Closed Bug 1318284 Opened 6 years ago Closed 6 years ago

Improve probes for Telemetry ping sending

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

The Event Telemetry work (bug 1286606) and other changes have significant impact on ping sizes.
One good proxy to see how this affects upload (for limited bandwidth etc.) would be TELEMETRY_SEND (the time it takes to upload a ping and get a response back).

We should tune the buckets for this to be more helpful and differentiate between success and failure.
I noticed that TELEMETRY_SEND measures the same time as TELEMETRY_PING:
https://dxr.mozilla.org/mozilla-central/rev/13f49da109ea460665ad27c8497cb1489548450c/toolkit/components/telemetry/TelemetrySend.jsm#835

We should kill that one.
This breaks up ping send times by success and failure, removes the redundant TELEMETRY_PING and adds test coverage.
Attachment #8812744 - Flags: review?(alessio.placitelli)
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
Summary: Improve TELEMETRY_SEND probe → Improve probes for Telemetry ping sending
Points: --- → 1
Comment on attachment 8812744 [details] [diff] [review]
Improve probes measuring Telemetry send times

Review of attachment 8812744 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, with the below addressed.

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +82,5 @@
>  
>  // The age of a pending ping to be considered overdue (in milliseconds).
>  const OVERDUE_PING_FILE_AGE = 7 * 24 * 60 * MS_IN_A_MINUTE; // 1 week
>  
> +function monotonicNow() {

This looks very similar to the one in TelemetrySession, but it has a different fall back policy. Maybe we can unify then and move it to TelemetryUtils.jsm?
Attachment #8812744 - Flags: review?(alessio.placitelli) → review+
(In reply to Alessio Placitelli [:Dexter] from comment #4)
> > +function monotonicNow() {
> 
> This looks very similar to the one in TelemetrySession, but it has a
> different fall back policy. Maybe we can unify then and move it to
> TelemetryUtils.jsm?

Those are both 5-line wrappers around nsITelemetry.msSinceProcessStart(), but with very different semantics.
We won't be able to easily change the one in TelemetrySession without planning for the data impact.
Every other refactoring would probably add more core around both uses than it would save us.
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edcd2fb87124
Improve probes measuring Telemetry send times. r=dexter
https://hg.mozilla.org/mozilla-central/rev/edcd2fb87124
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1323628
Whiteboard: [measurement:client] → [measurement:client] [measurement:client:uplift]
Comment on attachment 8812744 [details] [diff] [review]
Improve probes measuring Telemetry send times

Approval Request Comment
[Feature/Bug causing the regression]: Event Telemetry.
[User impact if declined]: Data of Event Telemetry behavior reaches us later, delaying our decision making for the project.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes, bug 1302671.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]:
The individual parts are:
* bug 1302663 - basic Telemetry implementation
* bug 1316810 - adjust event limits
* bug 1318284 - improve metrics that track effects on ping sending
* bug 1323628 - fix
* bug 1316281 - record search event in Telemetry
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's validated on Nightly and the changes are selective and relatively well understood.
[String changes made/needed]: No.

This is part of an uplift need for the initial Event Telemetry [1][2] implementation to Firefox 52.
We want to move this to Beta a little faster to see somewhat realistic data of an example event (search) coming in and verify that it matches our expectations.

I validated that this behaves as expected on Nightly over the last two weeks in bug 1302671.

1: https://docs.google.com/document/d/1hNuS9lUJMvMqgntZXbFA6xZBU9zBpQgo7x73-sXKRpI/
2: https://wiki.mozilla.org/Event_Telemetry
Attachment #8812744 - Flags: approval-mozilla-aurora?
Comment on attachment 8812744 [details] [diff] [review]
Improve probes measuring Telemetry send times

add event telemetry for aurora52
Attachment #8812744 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [measurement:client] [measurement:client:uplift] → [measurement:client]
You need to log in before you can comment on or make changes to this bug.