Closed
Bug 772552
Opened 12 years ago
Closed 12 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•12 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•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Updated•12 years ago
|
Assignee: nobody → aklotz
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #674352 -
Flags: review?(nfroyd)
Assignee | ||
Updated•12 years ago
|
Attachment #674352 -
Flags: feedback?(vdjeric)
Comment 3•12 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•12 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•12 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•12 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•12 years ago
|
||
This patch contains the variable name change.
Attachment #674464 -
Attachment is obsolete: true
Reporter | ||
Comment 8•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
Attachment #674823 -
Attachment is obsolete: true
Attachment #674823 -
Flags: review?(nfroyd)
Attachment #674875 -
Flags: review?(nfroyd)
Attachment #674875 -
Flags: feedback?(vdjeric)
Updated•12 years ago
|
Attachment #674875 -
Flags: feedback?(vdjeric) → feedback+
Reporter | ||
Comment 14•12 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•12 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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a346b585255
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 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
•