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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Finkle wants this for beta.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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/

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Bug 1270183 tracks making this available in the pipeline in the meta data of Telemetry pings.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Georg, do we want to document this somehow?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 8

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/fdece96f5cf5e05434dcedb60c11f689fdd126ff
Bug 1268525 - Add ping submission date to HTTP header. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/1f617b13309c81552fb495701476f0c05610792f
Bug 1268525 - review: Add test for non GMT time zone. r=me

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fdece96f5cf5
https://hg.mozilla.org/mozilla-central/rev/1f617b13309c

Comment 13

[Georg Fritzsche [:gfritzsche]]

Michael, can we uplift this to beta?

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 15

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 16

[Gerry Chang [:gchang]]

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.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/releases/mozilla-beta/rev/241b7e2f3155

Comment 20

[Carsten Book [:Tomcat]]

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

setting flag

Comment 21

[Sorina Florean [:sflorean]]

Hi, is there Steps to reproduce to verifying this bug?
Thanks!

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

To be explicit, the patch in this bug adds the date HTTP header to core ping uploads.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Frank, does the incoming data there look good?

Comment 25

[Frank Bertsch [:frank]]

Yup, this is working!