Add the client's current time to HTTP headers for core ping

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 49
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed, fennec47+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

We can use the desktop bug for guidance: https://bugzilla.mozilla.org/show_bug.cgi?id=1144778

Finkle wants this for beta.
(In reply to Michael Comella (:mcomella) from comment #0)
> Finkle wants this for beta.

Correction: whatever desktop is tracking (which is nothing atm). I'll leave it as beta for now to remind me to check frequently.
Assignee: nobody → michael.l.comella
Created attachment 8748914 [details]
MozReview Request: Bug 1268525 - Add ping submission date to HTTP header. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/50611/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50611/
Attachment #8748914 - Flags: review?(s.kaspari)
Comment on attachment 8748914 [details]
MozReview Request: Bug 1268525 - Add ping submission date to HTTP header. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50611/diff/1-2/
This doesn't rely on the reuploader implementation (bug 1243585) in theory, but it's landed on top of the code the reuploader is landed on.
Depends on: 1243585
Bug 1270183 tracks making this available in the pipeline in the meta data of Telemetry pings.
Georg, do we want to document this somehow?
Flags: needinfo?(gfritzsche)
Comment on attachment 8748914 [details]
MozReview Request: Bug 1268525 - Add ping submission date to HTTP header. r=sebastian

https://reviewboard.mozilla.org/r/50611/#review48035

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/util/TestDateUtil.java:27
(Diff revision 2)
> +        final TimeZone gmt = TimeZone.getTimeZone("GMT");
> +        final GregorianCalendar calendar = new GregorianCalendar(gmt, Locale.US);
> +        calendar.set(2011, Calendar.FEBRUARY, 1, 14, 0, 0);
> +        final String expectedDate = "Tue, 01 Feb 2011 14:00:00 GMT";

Does it make sense to add a test case with a non-GMT input date?
Attachment #8748914 - Flags: review?(s.kaspari) → review+
(In reply to Michael Comella (:mcomella) from comment #6)
> Georg, do we want to document this somehow?

Good point, it would be great to mention this in the core-ping.rst after the submission URL parameters.
Flags: needinfo?(gfritzsche)
https://reviewboard.mozilla.org/r/50611/#review48035

> Does it make sense to add a test case with a non-GMT input date?

Hell yeah it does. :)
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> (In reply to Michael Comella (:mcomella) from comment #6)
> > Georg, do we want to document this somehow?
> 
> Good point, it would be great to mention this in the core-ping.rst after the
> submission URL parameters.

bug 1271390.

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fdece96f5cf5
https://hg.mozilla.org/mozilla-central/rev/1f617b13309c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Michael, can we uplift this to beta?
Flags: needinfo?(michael.l.comella)
Comment on attachment 8748914 [details]
MozReview Request: Bug 1268525 - Add ping submission date to HTTP header. r=sebastian

(Already landed in 49)

Georg, can you elaborate on my response for "user impact if declined"?

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Georg requests this beta, probably for server-side analysis
[Describe test coverage new/current, TreeHerder]: Tested locally, in 49, & verified in bug 1271391.
[Risks and why]: Low – we do a set call to add a header to the http request, which we call from a pure function method. The only threats are 1) we crash from NullPointerException or similar, but that's always a threat, & 2) we don't include the date correctly (e.g. it's wrong, or we use the wrong header). However, we verified this so I don't expect issues.
[String/UUID change made/needed]: None
Flags: needinfo?(michael.l.comella) → needinfo?(gfritzsche)
Attachment #8748914 - Flags: approval-mozilla-beta?
status-firefox48: --- → affected
(In reply to Michael Comella (:mcomella) from comment #14)
> Comment on attachment 8748914 [details]
> MozReview Request: Bug 1268525 - Add ping submission date to HTTP header.
> r=sebastian
> 
> (Already landed in 49)
> 
> Georg, can you elaborate on my response for "user impact if declined"?
> 
> Approval Request Comment
> [Feature/regressing bug #]: N/A
> [User impact if declined]: Georg requests this beta, probably for
> server-side analysis

This give us data on the clock skew for Fennec, which in turn allows answering other quality and analysis questions.
Flags: needinfo?(gfritzsche)
Comment on attachment 8748914 [details]
MozReview Request: Bug 1268525 - Add ping submission date to HTTP header. r=sebastian

This patch is for data analysis. Take it in 48 beta 6.
Attachment #8748914 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
has problems to apply to beta:

grafting 343889:fdece96f5cf5 "Bug 1268525 - Add ping submission date to HTTP header. r=sebastian"
merging mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java
merging mobile/android/base/java/org/mozilla/gecko/util/DateUtil.java
merging mobile/android/base/moz.build
merging mobile/android/tests/background/junit4/src/org/mozilla/gecko/util/TestDateUtil.java
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/util/DateUtil.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/tests/background/junit4/src/org/mozilla/gecko/util/TestDateUtil.java! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(michael.l.comella)
Created attachment 8768606 [details] [diff] [review]
48 branch patch: Add ping submission date to HTTP header

A previous uplift seems to have added the DateUtil class & its test class so
we only needed to make the changes to PingResultDelegate, which is now the
CorePingResultDelegate in the latest beta tree.

MozReview-Commit-ID: 5elcQh2oyBG
(In reply to Michael Comella (:mcomella) from comment #19)
> https://hg.mozilla.org/releases/mozilla-beta/rev/241b7e2f3155

setting flag
status-firefox48: affected → fixed
Hi, is there Steps to reproduce to verifying this bug?
Thanks!
Flags: needinfo?(michael.l.comella)
(In reply to Sorina Florean [:sorina] from comment #21)
> Hi, is there Steps to reproduce to verifying this bug?
> Thanks!

In Beta & aurora, there is no way to redirect the pings to a new server so we can:
 1) Verify the telemetry server is receiving the data
 2) Sniff the outgoing packets with Wireshark or similar to ensure the HTTP header is set

I vote 1). Unfortunately, I'm not sure when the latest Beta will ship (I think they're spun on Mondays & propagated through Google Play within 48 hours? but archive.mozilla.org [1] says the latest one is from the 7th, which is 1 day after this was uplifted so I don't know!) so I'm not sure when we can test this.

Georg, once the Beta is made public (see above), can you verify the HTTP header is correctly set?

In nightly (50), in addition to the options above, you can change the "toolkit.telemetry.server" preference to point to a local server – see [2] for more.

However, 1) is still the most correct option because we're checking the results on the end-point where we use that data.

[1]: https://archive.mozilla.org/pub/mobile/releases/48.0b6/android-api-15/en-US/
[2]: https://wiki.mozilla.org/Mobile/Fennec/Android/Java_telemetry#Testing_production_builds
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(gfritzsche)
To be explicit, the patch in this bug adds the date HTTP header to core ping uploads.
Frank, does the incoming data there look good?
Flags: needinfo?(gfritzsche) → needinfo?(fbertsch)
Yup, this is working!
Flags: needinfo?(fbertsch)
You need to log in before you can comment on or make changes to this bug.