Closed
Bug 1253338
Opened 8 years ago
Closed 8 years ago
Add creation date to "core" ping
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox48 fixed, firefox49 fixed)
RESOLVED
FIXED
Firefox 49
People
(Reporter: gfritzsche, Assigned: mcomella)
References
Details
Attachments
(5 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
ahunt
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details |
58 bytes,
text/x-review-board-request
|
ahunt
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ahunt
:
review+
|
Details |
5.93 KB,
patch
|
Details | Diff | Splinter Review |
To power the engagement ratio, we will need to add a creation date to the "core" ping (i.e. date the ping was created on vs. the date it was submitted on). Otherwise, for pings that are not sent immediately, we will account for them on the wrong day.
Reporter | ||
Comment 1•8 years ago
|
||
The main motivation is to power the engagement ratio (bug 1240849) from this. Note that this is calculated on local time. To keep with the shorter form of properties used in the "core" ping, i propose adding: > "created": <ISO date string> Where the ISO date string has minutes of precision + timezone info, e.g.: "2016-03-03T17:55+01:00" A more compact representations could be e.g.: > "created": [<days since unix epoch>, <timezone offset in minutes>] E.g.: [24283735, 60] The values would add 24 bytes vs. 13 bytes respectively - do we care enough about 11 bytes to throw away human-readable and directly parseable date strings?
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(mark.finkle)
Reporter | ||
Comment 2•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #1) > The main motivation is to power the engagement ratio (bug 1240849) from this. > Note that this is calculated on local time. > ... > Where the ISO date string has minutes of precision + timezone info, e.g.: > "2016-03-03T17:55+01:00" We could cut this down to daily precision + timezone info, but i assume there might be other interesting questions to answer from this (ping latency until it is sent, distribution of sessions over the day, ...).
Comment 3•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #1) > The main motivation is to power the engagement ratio (bug 1240849) from this. > Note that this is calculated on local time. > > To keep with the shorter form of properties used in the "core" ping, i > propose adding: > > "created": <ISO date string> This is my vote. Simple and clean.
Flags: needinfo?(mark.finkle)
Comment 4•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #1) > The main motivation is to power the engagement ratio (bug 1240849) from this. > Note that this is calculated on local time. To clarify: are you saying that we use local day, not UTC day, for our calculations? This seems like a lot of painful complexity — now you get into DST, users traveling, etc. etc. What's the value? Profile creation times already aren't timezoned: they're just Unix timestamps. Is there a reason not to do the same thing here? > The values would add 24 bytes vs. 13 bytes respectively - do we care enough > about 11 bytes to throw away human-readable and directly parseable date > strings? In my experience, most software ends up working with (milli)seconds since epoch anyway, so going from that to that via a complex format like an ISO date string with TZ is a waste of time. 1457108150754 beats 2016-03-04T08:15-08:00 in every way, and I'm one shell command away from showing it in any format I like.
Reporter | ||
Comment 5•8 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #4) > (In reply to Georg Fritzsche [:gfritzsche] from comment #1) > > The main motivation is to power the engagement ratio (bug 1240849) from this. > > Note that this is calculated on local time. > > To clarify: are you saying that we use local day, not UTC day, for our > calculations? > > This seems like a lot of painful complexity — now you get into DST, users > traveling, etc. etc. What's the value? The engagement ratio is intentionally defined to use that, discussing that is probably better on that bug. The result here seems that we need to either submit UTC date + timezone offset or local dates (+ optionally TZ offset). > > Profile creation times already aren't timezoned: they're just Unix > timestamps. Is there a reason not to do the same thing here? For profile creation time we are not interested in when they were created in local time. For the ping creation date we are. > In my experience, most software ends up working with (milli)seconds since > epoch anyway, so going from that to that via a complex format like an ISO > date string with TZ is a waste of time. If we need local time or TZ info the ISO date string seems cleaner. Do we have a clean alternative that gives us the same info?
Comment 6•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #5) > If we need local time or TZ info the ISO date string seems cleaner. I agree with that; just seeing if we can change the rules upstream to simplify. If not, then ISO is fine.
Comment 7•8 years ago
|
||
Local day was an explicit request from the metrics team, so that we can do weekend/weekday reliably. Having activity by local date without timezone is really valuable. I'd argue that in desktop we should probably have used date instead of a truncated timestamp.
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #7) > Local day was an explicit request from the metrics team, so that we can do > weekend/weekday reliably. Having activity by local date without timezone is > really valuable. I'd argue that in desktop we should probably have used date > instead of a truncated timestamp. Unless there are issues with it i would keep the timezone - we can't reconstruct it later if we want to answer e.g. UTC date based questions. More than date precision would be useful to look at ping send latency or to look at what time during the day users are active. A precision of hours or minutes would be sufficient for that.
Reporter | ||
Comment 10•8 years ago
|
||
A simpler suggestion: * add a date field in local time: "created": <string> // YYYY-MM-DD * add a field with the timezone offset: "tz": <integer> // timezone offset in minutes This gets us the local date directly for the browser engagement ratio. The timezone offset field lets us convert that into UTC for other use-cases.
Comment 11•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #10) > A simpler suggestion: > * add a date field in local time: "created": <string> // YYYY-MM-DD > * add a field with the timezone offset: "tz": <integer> // timezone offset > in minutes > > This gets us the local date directly for the browser engagement ratio. > The timezone offset field lets us convert that into UTC for other use-cases. I don't have any problem with this proposal. It seems to handle our needs.
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #0) > To power the engagement ratio, we will need to add a creation date to the > "core" ping (i.e. date the ping was created on vs. the date it was submitted > on). > > Otherwise, for pings that are not sent immediately, we will account for them > on the wrong day. fwiw, we currently don't try to reupload failed pings so we only submit pings when they're created. As such, this doesn't sound high priority until bug 1243585 lands – I'll focus on that first.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → michael.l.comella
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #10) > * add a field with the timezone offset: "tz": <integer> // timezone offset > in minutes By timezone offset, do you strictly mean timezone offset (e.g. for PST, it's UTC-8, so -480)? Or does this relate to the current time of day?
Flags: needinfo?(gfritzsche)
Reporter | ||
Comment 14•8 years ago
|
||
Strictly the timezone offset in minutes.
Flags: needinfo?(gfritzsche)
Assignee | ||
Comment 15•8 years ago
|
||
I initially wrote the "ForGivenDate" variant for testing purposes - so I could verify the daylight savings time APIs worked the way I expected them to. However, I ended up using it in my next patch and leaving in the generic "for current time" variant because it seems useful. Review commit: https://reviewboard.mozilla.org/r/50885/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50885/
Attachment #8749341 -
Flags: review?(ahunt)
Attachment #8749342 -
Flags: review?(ahunt)
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50887/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50887/
Updated•8 years ago
|
Attachment #8749341 -
Flags: review?(ahunt) → review+
Comment 17•8 years ago
|
||
Comment on attachment 8749341 [details] MozReview Request: Bug 1253338 - Add DateUtil.getTimezoneOffsetInMinutes*. r=ahunt https://reviewboard.mozilla.org/r/50885/#review47877 ::: mobile/android/base/java/org/mozilla/gecko/util/DateUtil.java:44 (Diff revision 1) > + public static int getTimezoneOffsetInMinutes(@NonNull final TimeZone timezone) { > + return getTimezoneOffsetInMinutesForGivenDate(Calendar.getInstance(timezone)); > + } > + > + /** > + * Returns the time zone offset for the given date in minutes. The date makes a different due to daylight s/different/difference/ ?
Updated•8 years ago
|
Attachment #8749342 -
Flags: review?(ahunt) → review+
Comment 18•8 years ago
|
||
Comment on attachment 8749342 [details] MozReview Request: Bug 1253338 - Add creation date to core ping. r=ahunt https://reviewboard.mozilla.org/r/50887/#review47881
Assignee | ||
Comment 19•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51183/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51183/
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8749341 [details] MozReview Request: Bug 1253338 - Add DateUtil.getTimezoneOffsetInMinutes*. r=ahunt Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50885/diff/1-2/
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8749342 [details] MozReview Request: Bug 1253338 - Add creation date to core ping. r=ahunt Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50887/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8749842 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 23•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51211/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51211/
Attachment #8749874 -
Flags: review?(ahunt)
Attachment #8749842 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8749842 [details] MozReview Request: Bug 1253338 - Add docs for core ping creation date fields. r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51183/diff/1-2/
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8749842 [details] MozReview Request: Bug 1253338 - Add docs for core ping creation date fields. r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51183/diff/2-3/
Attachment #8749842 -
Attachment description: MozReview Request: Bug 1253338 - Add docs for core ping creation date fields. r=gfritzche → MozReview Request: Bug 1253338 - Add docs for core ping creation date fields. r=gfritzsche
Attachment #8749842 -
Flags: review?(gfritzsche)
Comment 26•8 years ago
|
||
Comment on attachment 8749874 [details] MozReview Request: Bug 1253338 - Increment core ping version number. r=ahunt https://reviewboard.mozilla.org/r/51211/#review47885
Attachment #8749874 -
Flags: review?(ahunt) → review+
Reporter | ||
Updated•8 years ago
|
Attachment #8749842 -
Flags: review?(gfritzsche) → review+
Reporter | ||
Comment 27•8 years ago
|
||
Comment on attachment 8749842 [details] MozReview Request: Bug 1253338 - Add docs for core ping creation date fields. r=gfritzsche https://reviewboard.mozilla.org/r/51183/#review48085 ::: toolkit/components/telemetry/docs/core-ping.rst:44 (Diff revision 3) > "profileDate": <pos integer>, // Profile creation date in days since > // UNIX epoch. > "defaultSearch": <string>, // Identifier of the default search engine, > // e.g. "yahoo". > "distributionId": <string>, // Distribution identifier (optional) > + "created": <string>, // date the ping was created in "yyyy-mm-dd" "... in local time" ::: toolkit/components/telemetry/docs/core-ping.rst:114 (Diff revision 3) > not be persisted to disk is to ensure the value doesn't change once we start > sending it: we only want to send consistent values. > > Version history > --------------- > +* v5: add ``created`` & ``tz`` Nit: "added"?
Assignee | ||
Comment 28•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cc20a3a1812d0acee33cbb9382f866fe877536c1 Bug 1253338 - Add DateUtil.getTimezoneOffsetInMinutes*. r=ahunt https://hg.mozilla.org/integration/fx-team/rev/cd0c3acb37e051c8d56961784582c060328cdb34 Bug 1253338 - Add creation date to core ping. r=ahunt https://hg.mozilla.org/integration/fx-team/rev/40958aebbb80cb96f275abfb9c88c1827b6ee9fa Bug 1253338 - Increment core ping version number. r=ahunt https://hg.mozilla.org/integration/fx-team/rev/6ad2696e1878da89486c58e353cd7734deba0d65 Bug 1253338 - Add docs for core ping creation date fields. r=gfritzsche
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cc20a3a1812d https://hg.mozilla.org/mozilla-central/rev/cd0c3acb37e0 https://hg.mozilla.org/mozilla-central/rev/40958aebbb80 https://hg.mozilla.org/mozilla-central/rev/6ad2696e1878
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 30•8 years ago
|
||
NI self: sounds like we want this ASAP (47?) for the engagement ratio.
Flags: needinfo?(michael.l.comella)
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8749341 [details] MozReview Request: Bug 1253338 - Add DateUtil.getTimezoneOffsetInMinutes*. r=ahunt This request applies to the first three landed commits in this bug (i.e. all the commits but the docs one). Note: this *will* need a branch patch for beta &, looking at the dependencies, it's likely we'll need a branch patch for aurora too. Approval Request Comment [Feature/regressing bug #]: Telemetry core ping [User impact if declined]: We will be unable to create the engagement ratios, which is an essential part of our dashboards – it'd be great to get these numbers ASAP. [Describe test coverage new/current, TreeHerder]: Tested locally, on Nightly for 9 days. Ideally, we'd also have validated the results we're getting back are sane (bug 1271391). [Risks and why]: Low. We add some stateless utilities and then use the utilities to put data into an existing data object for upload. It's unlikely but we could get bad data or, as always, we may have caused a NullPointerException somewhere. [String/UUID change made/needed]: None
Flags: needinfo?(michael.l.comella)
Attachment #8749341 -
Flags: approval-mozilla-beta?
Attachment #8749341 -
Flags: approval-mozilla-aurora?
Comment on attachment 8749341 [details] MozReview Request: Bug 1253338 - Add DateUtil.getTimezoneOffsetInMinutes*. r=ahunt Let's uplift to Aurora48. For Beta, given that we gtb Beta8 on Monday, I would prefer to let this ride the 48 train. At this point, I would like to only accept uplifts for critical recent regressions, severe stability, security, perf, mlk issues only. This is to minimize code churn and get ready to ship this thing in 2 weeks.
Attachment #8749341 -
Flags: approval-mozilla-beta?
Attachment #8749341 -
Flags: approval-mozilla-beta-
Attachment #8749341 -
Flags: approval-mozilla-aurora?
Attachment #8749341 -
Flags: approval-mozilla-aurora+
status-firefox48:
--- → affected
Comment 33•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/1645797a1315 https://hg.mozilla.org/releases/mozilla-aurora/rev/f3139434149b https://hg.mozilla.org/releases/mozilla-aurora/rev/ddc847fa5f2c
Comment 34•8 years ago
|
||
seems this is causing on aurora tier 2 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2643980&repo=mozilla-aurora not sure if this is expected
Flags: needinfo?(michael.l.comella)
Assignee | ||
Comment 35•8 years ago
|
||
It looks like the file paths changed from 49 to 48: comment 33 added a brand new pingbuilders/TelemetryCorePingBuilder.java to add in the desired changes while the actual file is in pings/TelemetryCorePingBuilder.java. Working on a branch patch.
Assignee | ||
Comment 36•8 years ago
|
||
fwiw, this doesn't break the main build because the newly added file is not specified in the moz.build file, but our gradle builds, which run in the tier 2 jobs, don't have a whitelist of files to compile.
Assignee | ||
Comment 37•8 years ago
|
||
MozReview-Commit-ID: 1IyEHlp1Oau
Assignee | ||
Comment 38•8 years ago
|
||
This patch is intended to land after the first commit that landed & replace the next two commits that landed: * first: 1645797a1315 * next: f3139434149b & ddc847fa5f2c MozReview-Commit-ID: 1IyEHlp1Oau
Assignee | ||
Updated•8 years ago
|
Attachment #8755584 -
Attachment is obsolete: true
Assignee | ||
Comment 39•8 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/714d6ee62adc remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/9850072a5a03
Flags: needinfo?(michael.l.comella)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•