Closed Bug 1374758 Opened 7 years ago Closed 7 years ago

Sync Ping "firm up" modifications: include environment.os, 'when' timestamp, etc

Categories

(Firefox for Android Graveyard :: Metrics, defect)

Product:
Firefox for Android Graveyard
Component:
Metrics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed, firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

Details

Attachments

(5 files)

Bug 1374758 - Move sync data format version to sync ping bundle
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1374758 - Add 'when' epoch timestamp to each sync ping
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1374758 - Rename bundleID to bundleId, for consistency with other platforms
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1374758 - Add 'os' object to Sync Ping bundle with basic OS information
59 bytes, text/x-review-board-request
nalexander
: review+
Details
sync-ping-firm-up-beta.patch
7.40 KB, patch
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Stemmed from desktop version of this work: Bug 1373093.
See Also: → 1374760
Additional changes to be made here, to combine a few small modifications into one bug:
- rename 'buildID' to 'buildId'
- add 'when' timestamp to individual sync objects
- move 'version' from individual sync objects to sync bundle payload object
Summary: Include OS data in sync ping → Sync Ping "firm up" modifications: include environment.os, 'when' timestamp, etc
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Comment on attachment 8880073 [details]
Bug 1374758 - Move sync data format version to sync ping bundle

https://reviewboard.mozilla.org/r/151420/#review156496

OK, this appears to agree with https://dxr.mozilla.org/mozilla-central/rev/92eb911c35da48907d326604c4c92cf55e551895/services/sync/tests/unit/sync_ping_schema.json.

What would it take to actually verify that Android Sync Pings conform to that JSON Schema?  Is that something that should be happening server side already?  Can we achieve that during testing?
Attachment #8880073 - Flags: review?(nalexander) → review+
Comment on attachment 8880074 [details]
Bug 1374758 - Add 'when' epoch timestamp to each sync ping

https://reviewboard.mozilla.org/r/151422/#review156498

Again, this is part of the documented JSON Schema and it's marked `required`.  Can we conform better?

Also, it's not clear to me _when_ `when` is: is it the latest possible time stamp (end of Sync), or the earliest possible (start of Sync), or something else entirely?  Generally I expect such things to either be leading edge or trailing edge.  Is that documented somewhere?  Does it matter?
Attachment #8880074 - Flags: review?(nalexander) → review+
Comment on attachment 8880075 [details]
Bug 1374758 - Rename bundleID to bundleId, for consistency with other platforms

https://reviewboard.mozilla.org/r/151424/#review156500
Attachment #8880075 - Flags: review?(nalexander) → review+
Comment on attachment 8880076 [details]
Bug 1374758 - Add 'os' object to Sync Ping bundle with basic OS information

https://reviewboard.mozilla.org/r/151426/#review156504

r- to make me look at this again.

This doesn't agree with the "core" ping at http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetryCorePingBuilder.java#77-92 or the documentation at http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/core-ping.html.

Why is this process so ad hoc?

::: mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetrySyncPingBundleBuilder.java:91
(Diff revision 1)
>          application.put("displayVersion", AppConstants.MOZ_APP_VERSION);
>          application.put("vendor", AppConstants.MOZ_APP_VENDOR);
>          application.put("xpcomAbi", AppConstants.MOZ_APP_ABI);
>          application.put("channel", AppConstants.MOZ_UPDATE_CHANNEL);
>  
> +        // Limited environment object, to help identify platforms easier. See Bug 1374758.

`locale` isn't mentioned at http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/sync-ping.html or at https://dxr.mozilla.org/mozilla-central/rev/92eb911c35da48907d326604c4c92cf55e551895/services/sync/tests/unit/sync_ping_schema.json.

How does this process work, coordinating all three clients and the service?
Attachment #8880076 - Flags: review?(nalexander) → review-
(In reply to Nick Alexander :nalexander from comment #7)
> Generally I expect such things to either be leading edge or trailing edge. [...] Does it matter?

Regarding 'when', I don't think it really matters. The main use of this is simply to plot trends over time. With that in mind, we _could_ be just sending a current date, and save a tiny bit on client bandwidth. iOS picks beginning of the "statssession", which I think means beginning of a sync. I'm not sure what desktop does. I don't think moving this timestamp to beginning of the sync will get us anything valuable, but it will means changes in many more places.
(In reply to Nick Alexander :nalexander from comment #9)
> Why is this process so ad hoc?

Fair question. This particular thing wasn't a concern while it was just desktop, and documentation generally is now lagging behind with the introduction of two more clients to the pool of reportees.

I certainly need to do a better job moving docs for this forward. At the very least, these changes _are_ coordinated across platforms, and should all land fairly close to each other.

> How does this process work, coordinating all three clients and the service?

Three clients land similar patches (I'm working on the other two atm), docs get updated to reflect the new state of the world, and dashboard relying on this data are updated.
grisha: so my substantive concern is that the OS/locale reporting in the "core" and "sync" pings is different.  Can you justify the differences or converge "sync" into the "core" format in the next iteration?  Thanks!
Flags: needinfo?(gkruglov)
(In reply to Nick Alexander :nalexander from comment #12)
> grisha: so my substantive concern is that the OS/locale reporting in the
> "core" and "sync" pings is different.  Can you justify the differences or
> converge "sync" into the "core" format in the next iteration?  Thanks!

Can you clarify what you mean by different? The values should be the same, but they are put together differently (os:{}, vs. os,osversion,locale). Is that what you're referring to?

I didn't really consider core ping as a consistency point at all - should I? As long as all clients reporting sync ping are using the same format, I'm fine with whatever sane looking formatting we pick. Mark has a patch up at Bug 1373093 which is doing the same thing.
Flags: needinfo?(gkruglov)
(In reply to :Grisha Kruglov from comment #13)
> (In reply to Nick Alexander :nalexander from comment #12)
> > grisha: so my substantive concern is that the OS/locale reporting in the
> > "core" and "sync" pings is different.  Can you justify the differences or
> > converge "sync" into the "core" format in the next iteration?  Thanks!
> 
> Can you clarify what you mean by different? The values should be the same,

The values are not the same -- the "core" ping includes more about the device, which I think will capture more of the segmentation you're going to actually want in the wild.

> but they are put together differently (os:{}, vs. os,osversion,locale). Is
> that what you're referring to?

Partially, yes.

> I didn't really consider core ping as a consistency point at all - should I?

As your reviewer, I immediately started to look for "what we did elsewhere".  I'm surprised the implementing team didn't.  Is this a place you want to differentiate?  I doubt it.

> As long as all clients reporting sync ping are using the same format, I'm
> fine with whatever sane looking formatting we pick. Mark has a patch up at
> Bug 1373093 which is doing the same thing.

I guess I am too.  I just see duplication and trivial changes that a) don't take into account what I expect will be hard-learned lessons from the past and b) don't make it easy to share ping analyses in a library, for example.  If that's the decision, roll on, I guess.
(In reply to Nick Alexander :nalexander from comment #14)
> As your reviewer, I immediately started to look for "what we did elsewhere".
> I'm surprised the implementing team didn't.  Is this a place you want to
> differentiate?  I doubt it.
> 
> > As long as all clients reporting sync ping are using the same format, I'm
> > fine with whatever sane looking formatting we pick. Mark has a patch up at
> > Bug 1373093 which is doing the same thing.
> 
> I guess I am too.  I just see duplication and trivial changes that a) don't
> take into account what I expect will be hard-learned lessons from the past
> and b) don't make it easy to share ping analyses in a library, for example. 
> If that's the decision, roll on, I guess.

I see. Inconsistencies like these will certainly make life harder in the future if we try to unify some processing, or use the pings together somehow, one way or another. The two pings are fairly orthogonal in a variety of ways: who's looking at them, how they're analyzed, etc - and that gives me hope that your fears won't materialize. The only thing that we're missing from the core ping that I anticipate to be useful at some point is device (model, manufacturer, etc) information - and primarily useful only for the android ping; we can always tack that on when and if the time comes.

For now the decision is to "roll on", unless you feel strongly that we should reconsider.
Comment on attachment 8880076 [details]
Bug 1374758 - Add 'os' object to Sync Ping bundle with basic OS information

https://reviewboard.mozilla.org/r/151426/#review159754

grisha and I have talked this over in the ticket and in person.  I still feel that the plurality of approaches will bite us eventually, but that day could be far away.

Roll on!
Attachment #8880076 - Flags: review- → review+
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50e84c3abf57
Move sync data format version to sync ping bundle r=nalexander
https://hg.mozilla.org/integration/autoland/rev/bcc6e922e248
Add 'when' epoch timestamp to each sync ping r=nalexander
https://hg.mozilla.org/integration/autoland/rev/28f34e8dd73c
Rename bundleID to bundleId, for consistency with other platforms r=nalexander
https://hg.mozilla.org/integration/autoland/rev/ebb2b7188913
Add 'os' object to Sync Ping bundle with basic OS information r=nalexander
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1308337

[User impact if declined]: No user impact. However, certain omissions in the Sync Ping as it landed in 55 make it harder to use than necessary. Specifically, tracking trends over time becomes very hard, as well as segmenting data effectively by platforms. These patches fix those issues, making our data much more useful.

[Is this code covered by automated tests?]: Tests touch this code, but not directly.

[Has the fix been verified in Nightly?]: Not yet, waiting for the next nightly Play Store build with this patch to land. Once that happens, I will confirm that newly submitted sync pings look correct and will update this bug.

[Needs manual test from QE? If yes, steps to reproduce]: N/A

[List of other uplifts needed for the feature/fix]: N/A

[Is the change risky?]: No

[Why is the change risky/not risky?]: Changes are very minor, changing how submitted json payloads are structured.

[String changes made/needed]: N/A
Attachment #8884022 - Flags: approval-mozilla-beta?
(In reply to Wes Kocher (:KWierso) from comment #20)
> I believe this caused android test failures like
> https://public-artifacts.taskcluster.net/UGMQpq_oT4WRCqWgPye-6w/0/public/
> android/unittest/officialAustralisDebug/index.html
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=112211200&repo=mozilla-
> central&lineNumber=3671

Well, that's terrible. I have completely missed the test updates for this. Patch incoming...
Flags: needinfo?(gkruglov)
Depends on: 1379025
(In reply to :Grisha Kruglov from comment #19)
> Created attachment 8884022 [details] [diff] [review]
> sync-ping-firm-up-beta.patch
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: Bug 1308337
> 
> [User impact if declined]: No user impact. However, certain omissions in the
> Sync Ping as it landed in 55 make it harder to use than necessary.
> Specifically, tracking trends over time becomes very hard, as well as
> segmenting data effectively by platforms. These patches fix those issues,
> making our data much more useful.
> 
> [Is this code covered by automated tests?]: Tests touch this code, but not
> directly.
> 
> [Has the fix been verified in Nightly?]: Not yet, waiting for the next
> nightly Play Store build with this patch to land. Once that happens, I will
> confirm that newly submitted sync pings look correct and will update this
> bug.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: N/A
> 
> [List of other uplifts needed for the feature/fix]: N/A
> 
> [Is the change risky?]: No
> 
> [Why is the change risky/not risky?]: Changes are very minor, changing how
> submitted json payloads are structured.
> 
> [String changes made/needed]: N/A

Uplift request is still valid, but patch from Bug 1379025 will need to be uplifted as well.
How did your tests of the last Play Store nightly build go?
Flags: needinfo?(gkruglov)
Bug 1373093 is not in 55, is the fennec version special somehow?
Does this really need uplift to 55 beta? We are heading into beta 9 of Nick, asking you too since you reviewed this. Thanks.
status-firefox55: --- → ?
Flags: needinfo?(nalexander)
(In reply to Julien Cristau [:jcristau] from comment #25)
> Bug 1373093 is not in 55, is the fennec version special somehow?

The patch that _really_ needs an uplift to 55 is addition of 'when' parameter. Tracking sync ping over time becomes very difficult without it.

If there's perceived risk around inclusion of the 'os' section or some of the other patches, I'm happy to split out the "really needs an uplift" bits. The risk should be very low for the whole series, however.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24)
> How did your tests of the last Play Store nightly build go?

Pings flowing from latest Nightlies look good.
Flags: needinfo?(gkruglov)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> Does this really need uplift to 55 beta? We are heading into beta 9 of Nick,
> asking you too since you reviewed this. Thanks.

I concur with Grisha in c#27.
Flags: needinfo?(nalexander)
Comment on attachment 8884022 [details] [diff] [review]
sync-ping-firm-up-beta.patch

OK, thanks. Let's uplift this for beta 9 (along with the tests in bug 1379025)
Attachment #8884022 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: