Add main process start time to pings that use event telemetry

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years 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)

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

2 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

2 years ago
Priority: -- → P2
Whiteboard: [measurement:client]
Assignee

Comment 3

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

Comment 5

2 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?
(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

2 years ago
Priority: P2 → P1
Assignee

Comment 7

2 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)
(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

2 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

Updated

2 years ago
See Also: → 1353307
Assignee

Comment 10

2 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

2 years ago
Assignee: nobody → gfritzsche
Assignee

Comment 11

2 years ago
Attachment #8854383 - Flags: review?(alessio.placitelli)
Assignee

Comment 12

2 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 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

2 years ago
Points: --- → 1
Assignee

Updated

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

Updated

2 years ago
Attachment #8854508 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8854509 - Flags: review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 16

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e4a0ac84712
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8854509 [details] [diff] [review]
Use hourly precision for sessionStartDate & subsessionStartDate

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

Updated

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