Add main process start time to pings that use event telemetry

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: sunahsuh, Assigned: gfritzsche)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
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)
(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

a year 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

a year ago
Priority: -- → P2
Whiteboard: [measurement:client]
(Assignee)

Comment 3

a year 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)
I think minutes would be fine for our purposes.
Flags: needinfo?(markh)

Comment 5

a year 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?
(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

a year ago
Priority: P2 → P1
(Assignee)

Comment 7

a year 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)
(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

a year 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)

Updated

a year ago
See Also: → bug 1353307
(Assignee)

Comment 10

a year 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

a year ago
Assignee: nobody → gfritzsche
(Assignee)

Comment 11

a year ago
Created attachment 8854383 [details] [diff] [review]
Use hourly precision for sessionStartDate & subsessionStartDate
Attachment #8854383 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 12

a year 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 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

a year ago
Points: --- → 1
(Assignee)

Comment 14

a year ago
Created attachment 8854508 [details] [diff] [review]
Use hourly precision for sessionStartDate & subsessionStartDate
(Assignee)

Updated

a year ago
Attachment #8854383 - Attachment is obsolete: true
Attachment #8854383 - Flags: feedback?(benjamin)
(Assignee)

Comment 15

a year ago
Created attachment 8854509 [details] [diff] [review]
Use hourly precision for sessionStartDate & subsessionStartDate
(Assignee)

Updated

a year ago
Attachment #8854508 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8854509 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 16

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e4a0ac84712
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 19

a year ago
Comment on attachment 8854509 [details] [diff] [review]
Use hourly precision for sessionStartDate & subsessionStartDate

data-r+
Attachment #8854509 - Flags: feedback?(benjamin) → feedback+
(Assignee)

Updated

a year ago
Blocks: 1355712
You need to log in before you can comment on or make changes to this bug.