Closed
Bug 1318284
Opened 8 years ago
Closed 8 years ago
Improve probes for Telemetry ping sending
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: gfritzsche, Assigned: gfritzsche)
References
(Blocks 1 open bug)
Details
(Whiteboard: [measurement:client])
Attachments
(1 file)
(deleted),
patch
|
Dexter
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
Our timeout client-side is 90 seconds:
https://dxr.mozilla.org/mozilla-central/rev/13f49da109ea460665ad27c8497cb1489548450c/toolkit/components/telemetry/TelemetrySend.jsm#68
So it seems sensible to change the bucket parameters to go up to 90k ms with 20 buckets here:
https://telemetry.mozilla.org/histogram-simulator/#low=1&high=90000&n_buckets=20&kind=exponential&generate=log-normal
Assignee | ||
Comment 3•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Summary: Improve TELEMETRY_SEND probe → Improve probes for Telemetry ping sending
Assignee | ||
Updated•8 years ago
|
Points: --- → 1
status-firefox52:
--- → affected
Comment 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
(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
Comment 8•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Updated•8 years ago
|
Whiteboard: [measurement:client] → [measurement:client] [measurement:client:uplift]
Assignee | ||
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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+
Assignee | ||
Comment 11•8 years ago
|
||
bugherder uplift |
Assignee | ||
Updated•8 years ago
|
Whiteboard: [measurement:client] [measurement:client:uplift] → [measurement:client]
You need to log in
before you can comment on or make changes to this bug.
Description
•