Send an HTTP Date request header with telemetry pings

RESOLVED FIXED in Firefox 48

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: rvitillo, Assigned: gfritzsche)

Tracking

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

4 years ago
Whiteboard: [unifiedTelemetry] [measurement:client]
Assignee

Updated

3 years ago
Priority: P3 → P4
Assignee

Updated

3 years ago
Blocks: TelemetryAdditions
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
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
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
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(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

3 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

3 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

3 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?
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+
You need to log in before you can comment on or make changes to this bug.