772552 - add telemetry for number of saved pings loaded at startup

Status: RESOLVED FIXED

Description

[Nathan Froyd [:froydnj]]

Would be interesting to know how much of a backlog we are accumulating.

Comment 1

[(dormant account)]

(In reply to Nathan Froyd (:froydnj) from comment #0)
> Would be interesting to know how much of a backlog we are accumulating.

This will be the most misleading stat of all :). Every startup we'd accumulate number of saved histograms...fail to send..next startup count on more saved histogram. Would make for a really funky distribution for machines that cant reach our telemetry servers for a few days.

Comment 2

[Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman)]

Created attachment 674352 [details] [diff] [review]
Changes to add savedPings field to telemetry

Comment 3

[Vladan Djeric (:vladan)]

Comment on attachment 674352 [details] [diff] [review]
Changes to add savedPings field to telemetry

>+  // Number of pings that have been successfully sent during this session.
>+  _pingsSentThisSession: 0,

Shouldn't this be a bool-type flag that indicates whether a successful ping was sent for the *CURRENT* browsing session?

>
> 
>     let data = null;
>     try {
>       data = i.next();
>     } catch (e if e instanceof StopIteration) {
>       finishPings(reason);
>+      if (this._pingsSentThisSession > 0) {
>+        this._pingsLoaded = 0;
>+        this._pingLoadsCompleted = 0;
>+      }
>       return;
>     }

I'm a bit unclear on this. If we are able to send only some of the saved pings, why shouldn't we try re-sending all the saved pings, e.g. at the next idle-daily?

>@@ -599,16 +611,17 @@ TelemetryPing.prototype = {
>     hping.add(new Date() - startTime);
> 
>     if (success) {
>       let file = this.saveFileForPing(ping);
>       try {
>         file.remove(true);
>       } catch(e) {
>       }
>+      this._pingsSentThisSession++;
>     }
>   },

I think w.r.t. savedPings field, we should only care about whether we sent a ping for the current browsing session


>diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js b/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
>--- a/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
>+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
>@@ -175,16 +175,17 @@ function checkPayloadInfo(payload, reaso
> function checkPayload(request, reason, successfulPings) {
>   let payload = decodeRequestPayload(request);
> 
>   checkPayloadInfo(payload, reason);
>   do_check_eq(request.getHeader("content-type"), "application/json; charset=UTF-8");
>   do_check_true(payload.simpleMeasurements.uptime >= 0);
>   do_check_true(payload.simpleMeasurements.startupInterrupted === 1);
>   do_check_eq(payload.simpleMeasurements.shutdownDuration, SHUTDOWN_TIME);
>+  do_check_true(payload.simpleMeasurements.savedPings >= 0);

Shouldn't this be savedPings == 0?

Comment 4

[Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman)]

Created attachment 674464 [details] [diff] [review]
Revised savedPings telemetry changes

Had a discussion with Vladan about this; basically the source of my confusion was that I had forgotten that multiple pings from the same session will clobber each other on the server.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 674464 [details] [diff] [review]
Revised savedPings telemetry changes

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

Looks fine to me, but this needs a test before it goes in.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +220,5 @@
>    _pingsLoaded: 0,
>    // The number of those requests that have actually completed.
>    _pingLoadsCompleted: 0,
> +  // Has any pings been successfully sent to the server during this session.
> +  _pingSent: false,

Nit: "Whether any pings have been successfully sent..."

Please also incorporate the booleanness of the variable into the name, something like _pingsHaveBeenSent.

@@ +609,5 @@
>        try {
>          file.remove(true);
>        } catch(e) {
>        }
> +      this._pingSent = true;

This only works because getPayloads always returns the payload for the current session before saved pings.  Can you make this conditional on whether we're sending a ping for the current session (or at least not a saved-sesion ping)?

Comment 6

[Vladan Djeric (:vladan)]

Comment on attachment 674464 [details] [diff] [review]
Revised savedPings telemetry changes

Looks good, just rename the variable as Nathan suggested.

> @@ +609,5 @@
> >        try {
> >          file.remove(true);
> >        } catch(e) {
> >        }
> > +      this._pingSent = true;
> 
> This only works because getPayloads always returns the payload for the
> current session before saved pings.  Can you make this conditional on
> whether we're sending a ping for the current session (or at least not a
> saved-sesion ping)?

I'm ok with _pingSent being set to "true" either on sending current session's ping or sending any ping. Both semantics make sense to me.

Comment 7

[Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman)]

Created attachment 674767 [details] [diff] [review]
Revised patch

This patch contains the variable name change.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Vladan Djeric (:vladan) from comment #6)
> > @@ +609,5 @@
> > >        try {
> > >          file.remove(true);
> > >        } catch(e) {
> > >        }
> > > +      this._pingSent = true;
> > 
> > This only works because getPayloads always returns the payload for the
> > current session before saved pings.  Can you make this conditional on
> > whether we're sending a ping for the current session (or at least not a
> > saved-sesion ping)?
> 
> I'm ok with _pingSent being set to "true" either on sending current
> session's ping or sending any ping. Both semantics make sense to me.

Yeah, thinking about it again, it probably doesn't make a difference.  I was getting confused because of the |reason != "saved-session"| check in getCurrentPayloadSessionAndSlug.  I see why that check is needed now.

Comment 9

[(dormant account)]

Comment on attachment 674767 [details] [diff] [review]
Revised patch

Note _pingHasBeenSent state is redundant, same thing can be accomplished by .getHistogramById("TELEMETRY_SUCCESS").snapshot().sum > 0

Comment 10

[Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman)]

Created attachment 674823 [details] [diff] [review]
Patch incorporating Taras' suggestion and test

This patch incorporates Taras' suggestion and includes the test. We test for a savedPings value of 1 because at this point in the test, we've already loaded a saved ping.

Comment 11

[Vladan Djeric (:vladan)]

Comment on attachment 674823 [details] [diff] [review]
Patch incorporating Taras' suggestion and test

># HG changeset patch
># Parent a517f7ea5befe078cd0b75ec1949c37f7f19027b
># User Aaron Klotz <[e-mail]>
>Bug 772662 - Adds telemetry for saved pings
>
>diff --git a/toolkit/components/telemetry/TelemetryPing.js b/toolkit/components/telemetry/TelemetryPing.js
>--- a/toolkit/components/telemetry/TelemetryPing.js
>+++ b/toolkit/components/telemetry/TelemetryPing.js
>@@ -505,16 +505,21 @@ TelemetryPing.prototype = {
>     if (Object.keys(this._slowSQLStartup.mainThread).length
> 	|| Object.keys(this._slowSQLStartup.otherThreads).length) {
>       payloadObj.slowSQLStartup = this._slowSQLStartup;
>     }
>     
>     for (let ioCounter in this._startupIO)
>       payloadObj.simpleMeasurements[ioCounter] = this._startupIO[ioCounter];
> 
>+    if (reason != "saved-session" ||
>+	Telemetry.getHistogramById("TELEMETRY_SUCCESS").snapshot().sum > 0) {


Nit: I think we usually align the condition on the 2ndline with the condition on the first line
e.g. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#743

No need to resubmit, I'll fix this before I land it

Comment 12

[(dormant account)]

Comment on attachment 674823 [details] [diff] [review]
Patch incorporating Taras' suggestion and test

+    if (reason != "saved-session" ||
+	Telemetry.getHistogramById("TELEMETRY_SUCCESS").snapshot().sum > 0) {
+      payloadObj.simpleMeasurements.savedPings = this._pingsLoaded;
+    }

wrap this getHistogramById thing in a try/catch to be safe. We don't want to end up in an inconsistent state where for some reason the TELEMETRY_SUCCESS histogram isn't sent, causes an exception and we break telemetry submissions.

Comment 13

[Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman)]

Created attachment 674875 [details] [diff] [review]
Patch including try/catch

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 674875 [details] [diff] [review]
Patch including try/catch

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

Works for me.  Sorry for the delay in getting back to you.

For the record, if you get an r+ on something and the review includes requests for minor changes, you generally do not need to ask for re-review.  The reviewer will generally ask to see the patch again post-changes if they really want to.

Comment 15

[Vladan Djeric (:vladan)]

Comment on attachment 674875 [details] [diff] [review]
Patch including try/catch

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a346b585255

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a346b585255