Send an HTTP Date request header with telemetry pings

RESOLVED FIXED in Firefox 48

Status

()

P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: rvitillo, Assigned: gfritzsche)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
2
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: [unifiedTelemetry] [measurement:client])

Attachments

(1 attachment, 1 obsolete attachment)

As discussed on IRC, we should add a submissionDate field to a ping with the timestamp just before a ping is been sent to the server. This field will allow us to approximate the clock skew (see https://bugzilla.mozilla.org/show_bug.cgi?id=1040741#c2)
(Assignee)

Comment 1

4 years ago
Benjamin mentioned that there is a better solution planned via HTTP submission headers (AFAIR).
Flags: needinfo?(benjamin)
(Assignee)

Updated

4 years ago
Blocks: 1122482
No longer blocks: 1120356

Comment 2

4 years ago
I don't think this should be part of the payload. My understanding is that there is already an HTTP header which can be used for this purpose, but if not we should deal with clock skew via a custom HTTP header and not part of the payload. Once we've saved the payload on the client we shouldn't modify it before sending.

NI?mreid to decide whether we need anything on the client for future clock skew metrics.
Flags: needinfo?(benjamin) → needinfo?(mreid)

Comment 3

4 years ago
The HTTP Date header I mentioned over in bug 1040741 is in the server response, so you could easily track the skew on the client based on that.

There is a standard Date header in the HTTP Request too, and if Firefox sends that header we can store that with the message when it is received to calculate skew on the server side.

I think we should probably do both - add a histogram of clock skew on the client based on the server responses, and make sure we're sending the Date header in the requests.
Flags: needinfo?(mreid)

Comment 4

4 years ago
ok, I'm going to morph this bug to be specifically about making sure that we send an HTTP Date request header with telemetry pings. Detecting clock skew on the client will probably be part of self-support but doesn't need to be filed yet.
Summary: Add the submission date to a ping. → Send an HTTP Date request header with telemetry pings
Priority: -- → P3
(Assignee)

Updated

3 years ago
Whiteboard: [unifiedTelemetry] [measurement:client]
(Assignee)

Updated

3 years ago
Priority: P3 → P4
(Assignee)

Updated

3 years ago
Blocks: 1121467
No longer blocks: 1122482
(Assignee)

Updated

3 years ago
Priority: P4 → P3
(Assignee)

Updated

3 years ago
OS: Mac OS X → All
Hardware: x86 → All

Comment 6

3 years ago
Per discussion on IRC, yes let's go with RFC 1123 format.
(Assignee)

Updated

3 years ago
Priority: P3 → P1
(Assignee)

Updated

3 years ago
Assignee: nobody → gfritzsche
(Assignee)

Updated

3 years ago
Points: --- → 2
(Assignee)

Comment 7

3 years ago
Created attachment 8741849 [details] [diff] [review]
Send an HTTP Date request header with telemetry pings

WIP, this is easy enough to do. I am doubtful that we need any more than minute precision for clock skew diagnosis though and nulling out the seconds would reduce fingerprinting possibilities.
(Assignee)

Comment 8

3 years ago
Rebecca, do you have an opinion on the required precision?
Is a precision of minutes enough?
Flags: needinfo?(rweiss)

Comment 9

3 years ago
I'd prefer us to do full precision for this. There's very little fingerprinting risk here.

Even better would be adding a function to necko/XHR to "please send Date header" so that we don't have that logic in multiple places.
(Assignee)

Comment 10

3 years ago
Ok, lets go with full precision then.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Even better would be adding a function to necko/XHR to "please send Date
> header" so that we don't have that logic in multiple places.

I don't see any other client code sending date headers and this is just one trivial line, so i don't think we should dive into Necko here (or at least do that in a different bug).

There was a related discussion around clock skew for bug 1264914 (and bug 712612), but that is a different use-case that utilizes "Date" headers sent from the server.
Flags: needinfo?(rweiss)
(Assignee)

Comment 11

3 years ago
Created attachment 8744347 [details] [diff] [review]
Send an HTTP Date request header with telemetry pings

This just trivially adds the "Date" header. I confirmed with bz that Date.prototype.toUTCString() should always return RFC 1123 format, even if the spec is a bit ambiguous here. Note that test_TelemetrySend.js is a bit fragile when adding tests to the end, but i dont think it makes sense to look into this before bug 1145188 lands.
Attachment #8744347 - Flags: review?(alessio.placitelli)
(Assignee)

Updated

3 years ago
Attachment #8741849 - Attachment is obsolete: true
Attachment #8744347 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Comment 13

3 years ago
I'm waiting with this for bug 1145188 to land to not delay it further.
Depends on: 1145188
(Assignee)

Comment 14

3 years ago
Once this landed, we have to think about visualization.
I think for now we should probably just make sure that this will be available in our re:dash instance (sql.telemetry.mozilla.org).

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b652d51edf7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Comment 17

3 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> Once this landed, we have to think about visualization.
> I think for now we should probably just make sure that this will be
> available in our re:dash instance (sql.telemetry.mozilla.org).

I filed bug 1270505 which will make this available via sql.t.m.o
(Assignee)

Comment 18

2 years ago
I still have to validate this, then flagging for Aurora as bsmedberg wants this on 48 for crash data analysis.
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 19

2 years ago
So, there are still submissions without "meta/Date" from recent build ids :
https://gist.github.com/georgf/21a2330d9c599a0f2267e00a891a4cd7

That affects 381 of 18505 pings, so about 2%. Those might just be clients with older source revisions submitting with newer build ids, so i think we should go ahead with the uplift.
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 20

2 years ago
Comment on attachment 8744347 [details] [diff] [review]
Send an HTTP Date request header with telemetry pings

Approval Request Comment
[Feature/regressing bug #]: Telemetry.
[User impact if declined]: We need this for client clock skew data, which is required for reasoning e.g. for crash analysis.
[Describe test coverage new/current, TreeHerder]: baked fine on nightly, automated test coverage.
[Risks and why]: Low risk, its a trivial change and has coverage.
[String/UUID change made/needed]: None.
Attachment #8744347 - Flags: approval-mozilla-aurora?
status-firefox48: --- → affected
Comment on attachment 8744347 [details] [diff] [review]
Send an HTTP Date request header with telemetry pings

More data, taking it.
Attachment #8744347 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 22

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e21705062473
status-firefox48: affected → fixed
You need to log in before you can comment on or make changes to this bug.