1144778 - Send an HTTP Date request header with telemetry pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Roberto Agostino Vitillo (:rvitillo)]

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)

Comment 1

[Georg Fritzsche [:gfritzsche]]

Benjamin mentioned that there is a better solution planned via HTTP submission headers (AFAIR).

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Mark Reid [: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.

Comment 4

[Benjamin Smedberg]

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.

Comment 5

[Georg Fritzsche [:gfritzsche]]

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

[Mark Reid [:mreid]]

Per discussion on IRC, yes let's go with RFC 1123 format.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: 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.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Rebecca, do you have an opinion on the required precision?
Is a precision of minutes enough?

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

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.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Attachment: 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.

Comment 12

[Georg Fritzsche [:gfritzsche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fec8949c5d41

Comment 13

[Georg Fritzsche [:gfritzsche]]

I'm waiting with this for bug 1145188 to land to not delay it further.

Comment 14

[Georg Fritzsche [:gfritzsche]]

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 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3b652d51edf7

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b652d51edf7

Comment 17

[Mark Reid [:mreid]]

(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

Comment 18

[Georg Fritzsche [:gfritzsche]]

I still have to validate this, then flagging for Aurora as bsmedberg wants this on 48 for crash data analysis.

Comment 19

[Georg Fritzsche [: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.

Comment 20

[Georg Fritzsche [: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.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

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

More data, taking it.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e21705062473