Closed
Bug 1144778
Opened 10 years ago
Closed 9 years ago
Send an HTTP Date request header with telemetry pings
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: rvitillo, Assigned: gfritzsche)
References
Details
(Whiteboard: [unifiedTelemetry] [measurement:client])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Dexter
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Benjamin mentioned that there is a better solution planned via HTTP submission headers (AFAIR).
Flags: needinfo?(benjamin)
Assignee | ||
Updated•10 years ago
|
Comment 2•10 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•10 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•10 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
Updated•9 years ago
|
Priority: -- → P3
Assignee | ||
Updated•9 years ago
|
Whiteboard: [unifiedTelemetry] [measurement:client]
Assignee | ||
Updated•9 years ago
|
Priority: P3 → P4
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Priority: P4 → P3
Assignee | ||
Updated•9 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 5•9 years ago
|
||
Should we just use the standard "Date" header here (with RFC 1123 format)?
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.18
https://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.3.1
Comment 6•9 years ago
|
||
Per discussion on IRC, yes let's go with RFC 1123 format.
Assignee | ||
Updated•9 years ago
|
Priority: P3 → P1
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gfritzsche
Assignee | ||
Updated•9 years ago
|
Points: --- → 2
Assignee | ||
Comment 7•9 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•9 years ago
|
||
Rebecca, do you have an opinion on the required precision?
Is a precision of minutes enough?
Flags: needinfo?(rweiss)
Comment 9•9 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•9 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•9 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•9 years ago
|
Attachment #8741849 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
Updated•9 years ago
|
Attachment #8744347 -
Flags: review?(alessio.placitelli) → review+
Assignee | ||
Comment 13•9 years ago
|
||
I'm waiting with this for bug 1145188 to land to not delay it further.
Depends on: 1145188
Assignee | ||
Comment 14•9 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).
Updated•9 years ago
|
Comment 16•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 17•9 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•9 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•9 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•9 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?
Updated•8 years ago
|
status-firefox48:
--- → affected
Comment 21•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•