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)
Benjamin mentioned that there is a better solution planned via HTTP submission headers (AFAIR).
Flags: needinfo?(benjamin)
Blocks: 1122482
No longer blocks: 1120356
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)
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
Whiteboard: [unifiedTelemetry] [measurement:client]
Priority: P3 → P4
Blocks: TelemetryAdditions
No longer blocks: 1122482
Priority: P4 → P3
OS: Mac OS X → All
Hardware: x86 → All
Per discussion on IRC, yes let's go with RFC 1123 format.
Priority: P3 → P1
Assignee: nobody → gfritzsche
Points: --- → 2
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.
Rebecca, do you have an opinion on the required precision?
Is a precision of minutes enough?
Flags: needinfo?(rweiss)
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.
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)
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)
Attachment #8741849 - Attachment is obsolete: true
Attachment #8744347 - Flags: review?(alessio.placitelli) → review+
I'm waiting with this for bug 1145188 to land to not delay it further.
Depends on: 1145188
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).
https://hg.mozilla.org/mozilla-central/rev/3b652d51edf7
Status: NEW → RESOLVED
Closed: 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
I still have to validate this, then flagging for Aurora as bsmedberg wants this on 48 for crash data analysis.
Flags: needinfo?(gfritzsche)
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)
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.