Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: sync server side metrics adr #1608

Merged
merged 8 commits into from
Nov 22, 2024

Conversation

taddes
Copy link
Contributor

@taddes taddes commented Oct 10, 2024

Description

Required ADR to validate approach of using Glean for server-side metrics to measure DAU.

The ADR will be created and stored in the syncstorage-rs repo and referenced in our service runbooks and documentation.

Testing

N/A

Issue(s)

Closes SYNC-4434.

@taddes taddes self-assigned this Oct 10, 2024
@taddes taddes marked this pull request as ready for review October 10, 2024 17:33
@taddes taddes requested review from pjenvey, jrconlin and ddurst October 10, 2024 17:36
jrconlin
jrconlin previously approved these changes Oct 10, 2024
@taddes taddes force-pushed the feat/SYNC-4434_ADR-server-side-metrics-DAU branch from a5647ae to 1a9e579 Compare October 14, 2024 21:14
Copy link

@ddurst ddurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be reduced, focused, and specified a bit more to explain why we want to make this change in this way.

@taddes taddes force-pushed the feat/SYNC-4434_ADR-server-side-metrics-DAU branch from a947792 to 795a37f Compare November 20, 2024 15:59
@taddes taddes changed the title feat: sync server side metrics adr docs: sync server side metrics adr Nov 20, 2024
@taddes
Copy link
Contributor Author

taddes commented Nov 20, 2024

Hey folks, have the updated ADR here for review as we've finalized the approach with Glean team

ddurst
ddurst previously approved these changes Nov 21, 2024
Copy link

@ddurst ddurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few notes, but overall I like it. r+wc


However, this left an addition decision to either implement our own custom Glean code to emit "Glean-compliant" output, or to contribute to the Glean team's `glean_parser` repository for server-side metrics. The `glean_parser` is used for all server implementations of Glean, since the SDK is only available for client-side applications. Currently, Rust is not supported.

The Glean team does/did not have capacity to implemented the Rust `glean_parser` feature, so we had to decide what is the best solution not only for this use case, but to consider possible future use cases. In consultation with the Glean team, it became clear avoiding our custom implementation and opting for the general-purpose `glean_parser` for Rust was the ideal solution.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/implemented/implement/

semantic nit: I'm not sure that the lack of capacity in the Glean team informs anything about future use cases.

The last sentence seems off to me. Something learned in consultation with the Glean team steered us to avoid a custom implementation, and that's why the glean parser route is chosen -- it's hinted at in "became clear" and such. I think we should just state plainly why the glean parser route was deemed better.

* Satisfaction of requirement to measure internal DAU metrics.
* Core of Glean's purpose is to measure user interactions and the rich metadata that accompanies it.
* Capacity for future expansion of application metrics within Sync beyond DAU.
* Prepares for implementation of same measurements in autopush, also using Glean.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the easy path (to me) is just to lose the bullet "Prepares for implementation of same measurements in autopush, also using Glean", as it's covered by the first bullet.

* Use of standardized Mozilla tooling.
* Establishment of team knowledge of using Glean.
* Establishment of server-side Rust best practices, leading to easier development for backend Rust applications.
* Transparency in data review process with consideration made to minimum collection of data.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still sounds to me like the "Transparency in data review" bullet is not a pro about this choice, but a pro about what data we're using. I'd lose it if it doesn't support the point.

jrconlin
jrconlin previously approved these changes Nov 21, 2024
* Satisfaction of requirement to measure internal DAU metrics.
* Core of Glean's purpose is to measure user interactions and the rich metadata that accompanies it.
* Capacity for future expansion of application metrics within Sync beyond DAU.
* Prepares for implementation of same measurements in autopush, also using Glean.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Plus the first pro listed notes that this work is a basis for future server work.

(I'll caution that you really don't have a repeatable framework until you've used it at least three times. So I'd caution that this is still very much developmental.)

* Use of standardized Mozilla tooling.
* Establishment of team knowledge of using Glean.
* Establishment of server-side Rust best practices, leading to easier development for backend Rust applications.
* Transparency in data review process with consideration made to minimum collection of data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the Glean process is compiling files that declare the data that is to be collected, how it will be consumed, and the parties that are doing so. Those files are then (normally) used to auto-generate the code. (Our implementation is still "alpha" so we are generating the initial code by hand, being mindful of how it should be templatized later.)

I believe that those files and processes do aid in both transparency and minimal data collection.

@taddes taddes dismissed stale reviews from jrconlin and ddurst via d4acca6 November 22, 2024 18:31
@taddes taddes force-pushed the feat/SYNC-4434_ADR-server-side-metrics-DAU branch from d4acca6 to 8d66414 Compare November 22, 2024 18:32
@taddes taddes merged commit 7e21154 into master Nov 22, 2024
8 checks passed
@taddes taddes deleted the feat/SYNC-4434_ADR-server-side-metrics-DAU branch November 22, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants