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.

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

This breaks up ping send times by success and failure, removes the redundant TELEMETRY_PING and adds test coverage.

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?

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

Comment on attachment 8812744 [details] [diff] [review] Improve probes measuring Telemetry send times add event telemetry for aurora52