Closed
Bug 1349324
Opened 7 years ago
Closed 7 years ago
Add main process start time to pings that use event telemetry
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bugzilla, Assigned: gfritzsche)
References
(Blocks 1 open bug)
Details
(Whiteboard: [measurement:client])
Attachments
(1 file, 2 obsolete files)
10.40 KB,
patch
|
gfritzsche
:
review+
benjamin
:
feedback+
|
Details | Diff | Splinter Review |
Some use cases of event telemetry benefit from knowing the (approximate) absolute time of the event. Since event timestamps are relative to the main process start time, we should add it to the main ping (and other ping types that want to use events)
Comment 1•7 years ago
|
||
(In reply to Sunah Suh [:sunahsuh] from comment #0) > (and other ping types that want to use events) Yeah, the sync ping will want this.
Assignee | ||
Comment 2•7 years ago
|
||
This is a good point, there is probably interest in that outside of events as well. We should make it clear though that this will be subject to clock issues (or try to fix it up through meta field use). We already have sessionStartDate & subsessionStartDate in [1] as ISO dates, they are just currently truncated to daily precision. How about we just make them more precise? Is a precision of full minutes sufficient? Or would we need seconds? 1: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/main-ping.html
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: [measurement:client]
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] [away March 24 - April 2] from comment #2) > We already have sessionStartDate & subsessionStartDate in [1] as ISO dates, > they are just currently truncated to daily precision. > How about we just make them more precise? > Is a precision of full minutes sufficient? > Or would we need seconds? Mark, can you answer to the required precision?
Flags: needinfo?(markh)
Comment 5•7 years ago
|
||
FWIW, we are explicitly trying to keep low granularity of absolute dates, because granular timestamps make pings/users much more identifiable. So my goal here as data steward is to pick something that is as coarse as possible: can we do this with hours?
Comment 6•7 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #5) > So my goal here as data steward is to pick something that is as coarse as > possible: can we do this with hours? My goal is to be able to order events from different pings, and multiple pings with the same process start time makes this tricky. Is there a middle ground - is something like max(current_hour, minute_last_session_ended) or similar possible?
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #6) > (In reply to Benjamin Smedberg [:bsmedberg] from comment #5) > > So my goal here as data steward is to pick something that is as coarse as > > possible: can we do this with hours? > > My goal is to be able to order events from different pings, and multiple > pings with the same process start time makes this tricky. Is there a middle > ground - is something like max(current_hour, minute_last_session_ended) or > similar possible? Wasn't looking at round-trip time for sync over devices for diagnostics also desired? That's where i thought you might need minutes or higher precision.
Flags: needinfo?(markh)
Comment 8•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #7) > Wasn't looking at round-trip time for sync over devices for diagnostics also > desired? > That's where i thought you might need minutes or higher precision. Yeah, I'd certainly *like* higher precision, but I think 1-hour granularity is fine for what we actually need today. There's no reason we couldn't revisit this later, right?
Flags: needinfo?(markh)
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #8) > Yeah, I'd certainly *like* higher precision, but I think 1-hour granularity > is fine for what we actually need today. There's no reason we couldn't > revisit this later, right? Right, lets go with hours for now then.
Assignee | ||
Comment 10•7 years ago
|
||
Mark, i'll use this bug to do this for the main ping. I filed bug 1353307 for adding this to the sync ping.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gfritzsche
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8854383 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8854383 [details] [diff] [review] Use hourly precision for sessionStartDate & subsessionStartDate Requesting data-review for this change, see the changes to the .rst file.
Attachment #8854383 -
Flags: feedback?(benjamin)
Comment 13•7 years ago
|
||
Comment on attachment 8854383 [details] [diff] [review] Use hourly precision for sessionStartDate & subsessionStartDate Review of attachment 8854383 [details] [diff] [review]: ----------------------------------------------------------------- This looks good with observations below addressed. ::: toolkit/components/telemetry/TelemetryUtils.jsm @@ +59,5 @@ > 0, 0, 0, 0); > }, > > /** > + * Takes a date and returns it trunctated to a date with hourly precision. nit: "truncated" ::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js @@ +1228,5 @@ > keyed.add("b", 1); > > // Trigger and collect environment-change ping. > gMonotonicNow = fakeMonotonicNow(gMonotonicNow + 10 * MILLISECONDS_PER_MINUTE); > + let startDay = TelemetryUtils.truncateToDays(now); Is |startDay| still being used? @@ +1246,5 @@ > Assert.equal(ping.payload.histograms[COUNT_ID].sum, 1); > Assert.equal(ping.payload.keyedHistograms[KEYED_ID]["a"].sum, 1); > > // Trigger and collect another ping. The histograms should be reset. > + startDay = TelemetryUtils.truncateToHours(now); Should this be |startHour = ...| ?
Attachment #8854383 -
Flags: review?(alessio.placitelli) → review+
Assignee | ||
Updated•7 years ago
|
Points: --- → 1
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8854383 -
Attachment is obsolete: true
Attachment #8854383 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8854508 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8854509 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 16•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4a0ac84712 Use hourly precision for sessionStartDate & subsessionStartDate. r=dexter
Keywords: checkin-needed
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8854509 [details] [diff] [review] Use hourly precision for sessionStartDate & subsessionStartDate I missed reflagging on the new patch. Requesting data review for this precision change, see the changes to the .rst file.
Attachment #8854509 -
Flags: feedback?(benjamin)
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4e4a0ac84712
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 19•7 years ago
|
||
Comment on attachment 8854509 [details] [diff] [review] Use hourly precision for sessionStartDate & subsessionStartDate data-r+
Attachment #8854509 -
Flags: feedback?(benjamin) → feedback+
You need to log in
before you can comment on or make changes to this bug.
Description
•