-
Notifications
You must be signed in to change notification settings - Fork 474
Conversation
components/service/glean/src/main/java/mozilla/components/service/glean/error/ErrorRecording.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the overall approach is good. As highlighted in mozilla-mobile/fenix#2749, I think most of the code from that PR should live here, with this component shipping its own pings.yaml
and metrics.yaml
files.
components/service/glean/src/main/java/mozilla/components/service/glean/Glean.kt
Outdated
Show resolved
Hide resolved
...ts/service/glean/src/main/java/mozilla/components/service/glean/private/CounterMetricType.kt
Outdated
Show resolved
Hide resolved
...rvice/glean/src/main/java/mozilla/components/service/glean/storages/CountersStorageEngine.kt
Outdated
Show resolved
Hide resolved
3c3ed6e
to
1a93d32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice, thanks Lina for bearing with us on this. Let me know if you'd like a detailed review on this by one of us.
OK, I think this is ready for a first round of review! Thanks for all your help with this, @Dexterp37! I can't assign reviewers, so paging @travis79 and @liuche, too. ☎️ For more context, the a-s PR is mozilla/application-services#1112. The existing Sync ping is also documented here; this pulls those fields into Glean pings. |
...nts/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt
Show resolved
Hide resolved
...orage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice! I'd take another pass after you change the metrics.yaml
to share metric definitions across the different pings.
...orage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt
Outdated
Show resolved
Hide resolved
...storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Lina for following up on this. I think I have an actionable proposal to make testing code a bit simpler and not rely on Glean SDK exposing more than required. Let me know what you think about that (I couldn't test it locally: is this PR requiring some other change somewhere else?).
...r/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt
Outdated
Show resolved
Hide resolved
...age-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt
Outdated
Show resolved
Hide resolved
components/service/glean/src/test/java/mozilla/components/service/glean/TestUtil.kt
Outdated
Show resolved
Hide resolved
components/service/glean/src/main/java/mozilla/components/service/glean/utils/TestUtils.kt
Outdated
Show resolved
Hide resolved
After looking at @Dexterp37's suggestion, I have to say that I agree with him that mocking would be much better solution for what you are trying to do. |
This depends on a yet-to-be-published version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to request one last change before the final r+ on this. I think we should do without the refactoring on the Glean side, not even for the resetGlean
. From a testing perspective, this should work just fine without it. See my comments below.
...nents/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/TestUtil.kt
Outdated
Show resolved
Hide resolved
...nents/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/TestUtil.kt
Outdated
Show resolved
Hide resolved
components/service/glean/src/main/java/mozilla/components/service/glean/Glean.kt
Outdated
Show resolved
Hide resolved
...nts/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt
Outdated
Show resolved
Hide resolved
...nts/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt
Outdated
Show resolved
Hide resolved
...age-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bearing with us -- once again, @Dexterp37 has the better grasp on external testing, and I agree with what he suggests (even though it goes against what I first suggested).
I hope to make this work part of a documentation example so that folks in the future doing this sort of thing have a template to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, pending tests passing on CI.
Thanks again for your patience with us as a trailblazer!
...nents/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/TestUtil.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great from a Glean POV, thanks @linacambridge ! I dropped a few non-blocking nits below :) Other passes won't require new reviews from us ;)
...nents/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/TestUtil.kt
Outdated
Show resolved
Hide resolved
...nents/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/TestUtil.kt
Show resolved
Hide resolved
components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt
Show resolved
Hide resolved
Request for data collection review formAll questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.
We would like to measure the performance and correctness of our Rust sync implementation. This includes collecting the time taken to sync each data type (currently history and bookmarks), incoming and outgoing record counts, any errors that occur (reporting sanitized error messages in a With the exception of the error string, which does not contain PII, we're submitting timings and counts only.
We need to understand how our new Sync implementation behaves in the wild. The Sync ping in Firefox Desktop and Firefox for iOS already exists, and has been extremely valuable in identifying and diagnosing Sync issues. This pull request collects the same information as the Sync ping, but ports its structure to Glean.
Server-side metrics are not sufficient to understand Sync performance (especially for each step of an engine sync), given that the bulk of the work happens on clients. Validation data can only be collected on the client, since Sync records are encrypted and opaque to the server. The Sync ping for Desktop and iOS provides some stats about Desktop, but, since all three still use different Sync implementations, can't be extrapolated to Fenix.
Android Components consumers, including Fenix, do not currently report any Sync telemetry.
Note that the data steward reviewing your request will characterize your data collection based on the highest (and most sensitive) category.
I want to permanently monitor this data. (Lina Cambridge)
All Sync users with Sync enabled. The data is not correlated to the
All.
All.
All.
No.
Users can opt-out by disabling telemetry, or signing out of Sync.
We will create queries in re:dash to monitor the engines, adding to our existing sync engine error/success (https://sql.telemetry.mozilla.org/dashboard/sync-leif-status-dashboard-wip) dashboards and engine error analysis notebook for Desktop (https://gist.github.com/mhammond/66684669e1478d65bd60446cf150c244).
See above.
No. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?
Yes, documented in metrics.yaml
- Is there a control mechanism that allows the user to turn the data collection on and off?
Yes, whatever data controls the consumer provides
- If the request is for permanent data collection, is there someone who will monitor the data over time?
Permanent data collection for sync data collection, but there are automated tests.
- Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Type 2, sync behavior of various pings
- Is the data collection request for default-on or default-off?
Based on whatever consumers set
- Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?
Hashed Firefox Account ID
-
Is the data collection covered by the existing Firefox privacy notice?
Yes -
Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**
No, has automated tests -
Does the data collection use a third-party collection tool? If yes, escalate to legal.
No
bb07051
to
2c99f87
Compare
This commit adds two Glean pings to the Storage-Sync browser component: one for history, and one for bookmarks. Both pings record the same set of base metrics, including incoming and outgoing counts, sync duration, the hashed FxA UID, and, most importantly, the failure reason if the store fails to sync. The bookmarks ping records additional validation data. The Glean schema is a flattened version of the Sync ping that's currently sent on Firefox Desktop and Firefox for iOS. Instead of sending a single ping with multiple syncs, each having multiple engines, we send one Glean ping per engine per sync. The pings are recorded directly in the component, so any app that consumes it should get pings for free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good!
Not sure why github isn't letting me land this - " Merging can be performed automatically once the requested changes are addressed. ", but AFAIK it's all good to go (and I don't see any unresolved change requests). @pocmo could you land this, please, with your super powers? |
🛬 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this has already been merged, but congrats Lina on being the first to integrate Glean into another component and thanks for helping us improve the docs and the process! Cheers!
Thanks for all your help getting this working! ❤️ |
This PR adds two Glean pings to the Storage-Sync browser component:
one for history, and one for bookmarks. Both pings record the same set
of base metrics, including incoming and outgoing counts, sync duration,
the hashed FxA UID, and, most importantly, the failure reason if the
store fails to sync. The bookmarks ping records additional validation
data.
The Glean schema is a flattened version of the Sync ping that's
currently sent on Firefox Desktop and Firefox for iOS. Instead of
sending a single ping with multiple syncs, each having multiple
engines, we send one Glean ping per engine per sync.
The pings are recorded directly in the component, so any app that
consumes it should get pings for free.
This depends on a not-yet-released version of a-s, so the tests won't pass until then.
Pull Request checklist