Add main process start time to pings that use event telemetry

RESOLVED FIXED in Firefox 55

Status

()

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)

(Reporter)

Description

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

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: → bug 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
Created attachment 8854383 [details] [diff] [review]
Use hourly precision for sessionStartDate & subsessionStartDate
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)

Comment 14

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

Updated

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

Comment 15

2 years ago
Created attachment 8854509 [details] [diff] [review]
Use hourly precision for sessionStartDate & subsessionStartDate
(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
Last Resolved: 2 years ago
status-firefox55: --- → fixed
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.