Closed Bug 1827949 Opened 2 years ago Closed 2 years ago

Add sync metrics as dependency to Firefox iOS

Categories

(Data Platform and Tools Graveyard :: Glean Platform, task, P3)

Product:
Data Platform and Tools Graveyard
Component:
Glean Platform
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: lougenia, Assigned: janerik)

Details

Attachments

(1 file)

PR#608: Update firefox_ios to get sync metrics from the syncmanager component
49 bytes, text/x-github-pull-request
Details | Review

To be filled by the requester

Application ID*: org.mozilla.ios.firefox (I think? This is the identifier for Firefox iOS release anyway)
Application Canonical Name: Firefox iOS
Description: Sync metrics for the Firefox iOS mobile browser
Data-review response link: https://github.com/mozilla-mobile/android-components/pull/5386#pullrequestreview-392363687
Repository URL: https://github.com/mozilla/application-services
Locations of metrics.yaml files (can be many):

  • components/sync_manager/metrics.yaml

Locations of pings.yaml files (can be many):

  • components/sync_manager/pings.yaml

Dependencies**:

  • glean-core

Retention Days***: 180

Notes and guidelines

* This is the identifier used to initialize Glean (or the id used on the store on Android and Apple devices).

** Dependencies can be found in the Glean repositories. Each dependency must be listed explicitly. For example, the default Glean probes will only be included if glean itself is a dependency.

*** Number of days that raw data will be retained. A good default is 180. We can change this later to accommodate longer retention periods, though we cannot recover data that is past the retention period (for example, we cannot recover data that is 200 days old if your retention period is 180 days).

Need additional help?

If you need new dependencies, please file new bugs for them, separately from this one. For any questions, ask in the #glean channel.

To be filled by the Glean team

Application friendly name: my_app_name

The following are only required for products requiring encryption:

Document namespace: my-app-name

Please NI Operations on this bug to request the creation of encryption keys and an analysis project.

FYI: These are metrics that were originally defined for the Fenix app and were copied into the application services repo to be used for the Firefox iOS app. This most likely just requires updating the URLs here but I created this ticket in the event more work is needed.

Hey Jan-Erik, any chance you could double check this?

Flags: needinfo?(jrediger)

Sure.

This is not a new app really, so the form doesn't apply.
@Lougenia: Can you explain in more detail what definitions moved from where to where?
And where are they supposed to be used?

Flags: needinfo?(jrediger) → needinfo?(lougenia)

Edited the title, hope I understood correctly what's asked.

Summary: Enable new Glean App `my.app_id` → Add sync metrics as dependency to Firefox iOS

:janerik The metrics.yaml and pings.yaml files used by android components were copied into application services for use in the Firefox iOS app.

So these metrics aren't being moved they're being copied. At some future date we plan to move the android components files into application services so we can consolidate the mobile sync metrics, but that's future work.

Apologies for the confusion, and please let me know if you need any additional information.

Flags: needinfo?(lougenia) → needinfo?(jrediger)

Nope, understood now. I'll see that I get to it next week.

Assignee: nobody → jrediger
Flags: needinfo?(jrediger)
Priority: -- → P1

It looks like some (all?) of these metrics are already defined in firefox-ios directly here: https://github.com/mozilla-mobile/firefox-ios/blob/main/Sync/metrics.yaml
Comparing the metrics in a-s/components/sync_manager/metrics.yaml and firefox-ios/Sync shows some significant differences.

Unfortunately that means it's a bit of a hassle to untangle this and get the correct data.

Is that sync-manager component already in a release and used in Firefox iOS?
If not what's the timeline to get there?

Flags: needinfo?(lougenia)

The a-s sync-manager component is currently being used in firefox-ios beta, it is not yet available in release. These changes will be introduced in release via a Nimbus experiment. My plan was to start the experiment once the new sync telemetry was available (which hopefully will be in v113 assuming this PR is merged soon).

For a bit of context, the current firefox-ios sync metrics you linked to were created as a short-term way to have some telemetry around the integration of the tabs component until Glean in rust was available. So those metrics have unfortunately outlived their utility. For example the failure reason values collected in the current metrics don't map to the ones sync manager reports so there's some hacky code I created to adapt them.

Flags: needinfo?(lougenia)

So for context (and because I'd like to hand this over:

Firefox iOS currently has metrics defined in Sync/metrics.yaml.
E.g. sync.sync_uuid.
These are sent in temp-* custom pings, defined in Sync/pings.yaml
probe-scraper knows about them as direct files in the firefox-ios app, defined here.

The new files are in a-s here:

These differ in the details and in the list of pings metrics are sent in.
We probably want this as a new library defined in probe-scraper, so that Firefox iOS can depend on that.

There are also metrics and pings defined in a-c:

I have not double-checked if they already match.

Steps that we will need to take:

  • Create a new lib sync-manager in probe-scraper with those new files
  • Add it as a dependency to firefox-ios
  • Remove the now obsolete files from the metrics/pings files list
  • Check if we will run into "duplicate metrics defined" errors. :relud might be a good point of contact for assistance
  • When this is all landed check that we get new data correctly
  • Later: check if the Glean Dictionary correctly picks up on the new location of metrics

I took a closer look to see what issue we will run into.
Overall the new file in a-s is a near exact copy of what is in a-c.
Minor difference: the notification_email: sync-core@ vs sync-team@. Not sure if both work, but that can always be changed afterwards as well.

Comparing the new a-s file and the Firefox iOS file there's some differences:

  • Newly added or removed metrics
    • These are all fine
  • Changes in pings: No more temp- prefixes.
    • The pings are thus newly defined, that overall is ok.
  • Differences in expires
    • Changed from timed expire to never
    • This is fine (if the data-review covers it)
  • Differences in description, bugs, data_reviews
    • These are all fine and can also be edited later as well

The one big breaking change is changing several *.failure_reason metrics from labeled_counter to labeled_string.
This is usually what would break.
Now the good thing is that these are all in new pings, so technically there's no clash with the same-named-but-different-type metrics from the older temp-* pings.
But I don't think our tooling is able to understand that.

I got the probe-scraper change up here: https://github.com/mozilla/probe-scraper/compare/main...badboy:probe-scraper:1827949/sync-manager

:relud asking for your expertise here

Will this clash?
A naive python3 -m probe_scraper.runner --dry-run --cache-dir tmp/cache --out-dir tmp/out --glean --glean-repo firefox-ios-release --glean-repo sync-manager now works just fine.

But with the push model and updates and stuff and also the schema generation happen some place else I'm not entirely sure if this will pass as is.
Can you chime in? Or let me know how to best simulate this for testing?

Flags: needinfo?(dthorn)

let me know how to best simulate this for testing?

the command i ran to test is:

python3 -m probe_scraper.runner --dry-run --cache-dir tmp/cache --out-dir tmp/out --glean --glean-repo firefox-ios-release --glean-repo sync-manager --update --glean-limit-date=2023-04-14 --output-bucket=gs://probe-scraper-prod-artifacts/

compared to your command this adds --update --glean-limit-date=2023-04-14 --output-bucket=gs://probe-scraper-prod-artifacts/. 2023-04-14 was chosen as the last date when sync-manager files were modified. output bucket is necessary to pull prod state for doing duplicate metric checks.

Will this clash?

yes:

Glean has detected duplicated metric identifiers coming from the product 'firefox-ios-dev'.

- 'bookmarks_sync.failure_reason' defined more than once in firefox-ios-dev, sync-manager
- 'bookmarks_sync.incoming' defined more than once in firefox-ios-dev, sync-manager
- 'bookmarks_sync.outgoing' defined more than once in firefox-ios-dev, sync-manager
- 'bookmarks_sync.uid' defined more than once in firefox-ios-dev, sync-manager
- 'history_sync.failure_reason' defined more than once in firefox-ios-dev, sync-manager
- 'history_sync.incoming' defined more than once in firefox-ios-dev, sync-manager
- 'history_sync.outgoing' defined more than once in firefox-ios-dev, sync-manager
- 'history_sync.uid' defined more than once in firefox-ios-dev, sync-manager
- 'logins_sync.failure_reason' defined more than once in firefox-ios-dev, sync-manager
- 'logins_sync.incoming' defined more than once in firefox-ios-dev, sync-manager
- 'logins_sync.outgoing' defined more than once in firefox-ios-dev, sync-manager
- 'logins_sync.uid' defined more than once in firefox-ios-dev, sync-manager
- 'sync.failure_reason' defined more than once in firefox-ios-dev, sync-manager
- 'sync.sync_uuid' defined more than once in firefox-ios-dev, sync-manager
- 'tabs_sync.failure_reason' defined more than once in firefox-ios-dev, sync-manager
- 'tabs_sync.incoming' defined more than once in firefox-ios-dev, sync-manager
- 'tabs_sync.outgoing' defined more than once in firefox-ios-dev, sync-manager
- 'tabs_sync.uid' defined more than once in firefox-ios-dev, sync-manager
Flags: needinfo?(dthorn)

:janerik if configuring this will take a lot of time and effort, I can hold off adding this new telemetry until the sync manager experiment is over. We were hoping to have better telemetry around the sync manager changes but if the Nimbus experiment (and consequently needing these two sets of metrics to coexist) complicates things we can add the new metrics after the experiment. That way the new metrics can be added when the old metrics are being removed.

Flags: needinfo?(jrediger)

That way the new metrics can be added when the old metrics are being removed.

the metrics will still clash even after they are removed

the metrics will still clash even after they are removed

That's unfortunate. However, waiting to address it then will at least unblock the sync manager experiment. I was planning for starting it in mid-May.

Flags: needinfo?(dthorn)
Flags: needinfo?(dthorn)

(In reply to Lougenia Bailey from comment #12)

:janerik if configuring this will take a lot of time and effort, I can hold off adding this new telemetry until the sync manager experiment is over. We were hoping to have better telemetry around the sync manager changes but if the Nimbus experiment (and consequently needing these two sets of metrics to coexist) complicates things we can add the new metrics after the experiment. That way the new metrics can be added when the old metrics are being removed.

You will have to rename the metrics. It would be enough to use a different category (e.g. bookmarks_sync_v2.failure_reason, or sync.bookmarks.*) or change the metric name (adding a _v2 suffix at the name).
This is dissatisfying, I know, but right now we won't be able to support this any differently.

If necessary we might be able to build views on top of the generated tables to use different column names to make querying between the different apps more consistent (but keep in mind that this requires some manual work, so let us know early if that's needed).

Flags: needinfo?(jrediger)

forgot to tag you, so you see this.

Flags: needinfo?(lougenia)

Thanks for looking into this. I think I'm going to revert my Firefox iOS telemetry changes and use the old telemetry with the sync manager changes. Once the sync manager experiment has ended and the sync manager changes are in release, I'll make the renaming changes you've suggested and add the new telemetry back to iOS.

FYI between the time that my original PR and my revision PR hit, telemetry will be reported using the new yet to be configured metrics. Hopefully any adverse impact will be minimal.

Flags: needinfo?(lougenia) → needinfo?(jrediger)

That's ok. We haven't landed anything in probe-scraper, so our tooling is unaware of this data and thus will simply drop it.
If that shows up in some of our weekly monitoring I can make sure no one gets concerned.

I'm gonna keep this bug open, but drop assignment/priority. Once you're ready you can notify us here to bring it back.

Assignee: jrediger → nobody
Flags: needinfo?(jrediger)
Priority: P1 → P3

Sounds good! I'll be back around in a couple of weeks after I finish renaming the metrics. Thanks again!!

Well instead of being back in a couple of weeks I guess I meant a couple of months 😅. In any event we've renamed the new metrics we created in application services (see here for details) and I've created a PR to use those metrics to report the Firefox iOS sync telemetry here. Let me know if there's anything else I need to do to proceed with this work. Sorry for the delay!

Flags: needinfo?(jrediger)

I added the probe-scraper PR.
The "Glean probe-scraper / glean-probe-scraper" task on your firefox-ios PR will fail until we land that. Merging that will make any other PRs still using the old code fail though.
So I propose best to merge your PR first, land the probe-scraper one next and after that CI will be green again.

Assignee: nobody → jrediger
Flags: needinfo?(jrediger)

Thanks, Jan-Erik! I'll open my PR for review and update this bug once my iOS PR is merged.

FYI: My iOS PR has been merged.

Flags: needinfo?(jrediger)

badboy merged PR #608: "Bug 1827949 - Update firefox_ios to get sync metrics from the syncmanager component" in 04e1211.

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(jrediger)
Resolution: --- → FIXED
Product: Data Platform and Tools → Data Platform and Tools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: