Closed
Bug 772552
Opened 13 years ago
Closed 13 years ago
add telemetry for number of saved pings loaded at startup
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: froydnj, Assigned: bugzilla)
References
Details
Attachments
(1 file, 4 obsolete files)
Would be interesting to know how much of a backlog we are accumulating.
Comment 1•13 years ago
|
||
(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.
![]() |
Reporter | |
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
![]() |
Reporter | |
Updated•13 years ago
|
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Updated•13 years ago
|
Assignee: nobody → aklotz
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #674352 -
Flags: review?(nfroyd)
Assignee | ||
Updated•13 years ago
|
Attachment #674352 -
Flags: feedback?(vdjeric)
Comment 3•13 years ago
|
||
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?
Attachment #674352 -
Flags: feedback?(vdjeric) → feedback-
Assignee | ||
Comment 4•13 years ago
|
||
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.
Attachment #674352 -
Attachment is obsolete: true
Attachment #674352 -
Flags: review?(nfroyd)
Attachment #674464 -
Flags: review?(nfroyd)
Attachment #674464 -
Flags: feedback?(vdjeric)
![]() |
Reporter | |
Comment 5•13 years ago
|
||
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)?
Attachment #674464 -
Flags: review?(nfroyd) → review+
Comment 6•13 years ago
|
||
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.
Attachment #674464 -
Flags: feedback?(vdjeric) → feedback+
Assignee | ||
Comment 7•13 years ago
|
||
This patch contains the variable name change.
Attachment #674464 -
Attachment is obsolete: true
![]() |
Reporter | |
Comment 8•13 years ago
|
||
(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•13 years ago
|
||
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
Assignee | ||
Comment 10•13 years ago
|
||
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.
Attachment #674767 -
Attachment is obsolete: true
Attachment #674823 -
Flags: review?(nfroyd)
Attachment #674823 -
Flags: feedback?(vdjeric)
Comment 11•13 years ago
|
||
Comment on attachment 674823 [details] [diff] [review]
Patch incorporating Taras' suggestion and test
># HG changeset patch
># Parent a517f7ea5befe078cd0b75ec1949c37f7f19027b
># User Aaron Klotz <aklotz@mozilla.com>
>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
Attachment #674823 -
Flags: feedback?(vdjeric) → feedback+
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #674823 -
Attachment is obsolete: true
Attachment #674823 -
Flags: review?(nfroyd)
Attachment #674875 -
Flags: review?(nfroyd)
Attachment #674875 -
Flags: feedback?(vdjeric)
Updated•13 years ago
|
Attachment #674875 -
Flags: feedback?(vdjeric) → feedback+
![]() |
Reporter | |
Comment 14•13 years ago
|
||
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.
Attachment #674875 -
Flags: review?(nfroyd) → review+
Comment 15•13 years ago
|
||
Comment on attachment 674875 [details] [diff] [review]
Patch including try/catch
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a346b585255
Attachment #674875 -
Flags: checkin+
Comment 16•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•