Nimbus: add enrollment_status metric #5857

jeddai · 2023-10-05T21:19:33Z

https://mozilla-hub.atlassian.net/browse/EXP-3827

This PR does a number of things, collectively a part of the above ticket.

Define and records the enrollment_status glean metric
Add testing for enrollment_status metric
Create callback handler for host metrics
Update iOS/Kotlin SDKs to define/use/supply the callback handler

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-10-05T21:29:30Z

Codecov Report

Attention: Patch coverage is 0% with 76 lines in your changes missing coverage. Please review.

Project coverage is 36.73%. Comparing base (b7efb70) to head (0a1300c).
Report is 573 commits behind head on main.

Files with missing lines	Patch %	Lines
components/nimbus/src/enrollment.rs	0.00%	32 Missing ⚠️
components/nimbus/src/metrics.rs	0.00%	29 Missing ⚠️
components/nimbus/src/stateful/nimbus_client.rs	0.00%	9 Missing ⚠️
components/nimbus/src/stateful/dbcache.rs	0.00%	4 Missing ⚠️
components/nimbus/src/error.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5857      +/-   ##
==========================================
- Coverage   36.81%   36.73%   -0.09%     
==========================================
  Files         347      348       +1     
  Lines       33447    33522      +75     
==========================================
  Hits        12313    12313              
- Misses      21134    21209      +75

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jeddai

Pre-reviewing myself to share some thoughts I have already

components/nimbus/Cargo.toml

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt

jeddai · 2023-10-05T21:30:23Z

components/nimbus/src/enrollment.rs

+impl EnrollmentStatus {
+    pub fn name(&self) -> String {
+        match self {
+            EnrollmentStatus::Enrolled { .. } => "Enrolled",
+            EnrollmentStatus::NotEnrolled { .. } => "NotEnrolled",
+            EnrollmentStatus::Disqualified { .. } => "Disqualified",
+            EnrollmentStatus::WasEnrolled { .. } => "WasEnrolled",
+            EnrollmentStatus::Error { .. } => "Error",
+        }
+        .into()
+    }
+}


So I couldn't use the fmt::Debug thing here because it ends up including the values associated with each enum and that

makes testing difficult at best

shouldn't be sent to Glean according to the metric spec

If there's a better way I could've done this please let me know!

You might be better keeping this as an implementation of fmt::Display to keep it consistent with the other enums— notwithstanding my previous comment about using Display to serialize.

The associated values can still be displayed with Debug.

jeddai · 2023-10-05T21:31:46Z

components/nimbus/src/metrics.rs

+}
+
+#[derive(Serialize, Deserialize, Clone)]
+#[serde(remote = "EnrollmentStatusExtra")]


This is nice because even though we have to manually re-define the struct here, the serde-remote piece does at least verify that they have the same members.

jeddai · 2023-10-05T21:32:30Z

components/nimbus/src/metrics.rs

+        let mut reason_value: Option<String> = None;
+        let mut error_value: Option<String> = None;
+        match &enrollment.status {
+            EnrollmentStatus::Enrolled { reason, branch, .. } => {


So idk if there's a way I could cast reason to a string from its enum type right here but it'd be nice because I could fold two of these branches up into one.

components/nimbus/src/metrics.rs

components/nimbus/tests/common/mod.rs

jeddai · 2023-10-05T22:00:48Z

Request for data collection review form

What questions will you answer with this data?
What are the enrollment statuses of all recipes a given user is enrolled in when recipes are finished being applied?
Why does Mozilla need to answer these questions?
This data is required for furthering the development of Nimbus. Specifically, it will help data science resolve a current issue where *"we get telemetry from users who do not enroll in deliveries that we expect them to"

* ref. telemetry proposal document, linked in the ticket linked on this PR.

What alternative methods did you consider to answer these questions? Why were they not sufficient?
None, no solution is sufficient unless the data collected in this metric is collected.
We chose to combine what would have been a significant number of new metrics into a single enrollment_status metric.
Can current instrumentation answer these questions?
No
List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki.

Measurement Description	Data Collection Category	Tracking Bug #
Enrollment Status	Technical data	https://mozilla-hub.atlassian.net/browse/EXP-3827

Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way.
The documentation can be found in the proposal document linked in the ticket above (internal only), the metrics.yaml on this PR, and the Glean dictionary.
How long will this data be collected? Choose one of the following:

I want to permanently monitor this data.
- [email protected]
- [email protected]

What populations will you measure?
All populations that use the Nimbus SDK. All of Firefox Android, Firefox iOS, and ultimately all tools using the stateless Nimbus SDK as well.
If this data collection is default on, what is the opt-out mechanism for users?
Nimbus has internal tooling for opting out, it is up to the implementing applications to use this tooling correctly.
Please provide a general description of how you will analyze this data.
We'll analyze it in Jetstream and it will be used to create dashboards for experiment health as well as additional data.
Where do you intend to share the results of your analysis?
They will be shared internally to Mozilla through the Experimenter console as well as ad-hoc through queries against the raw Glean data.
Is there a third-party tool (i.e. not Glean or Telemetry) that you are proposing to use for this data collection? If so:
No

data-review?@eliserichards

eliserichards · 2023-10-05T22:08:23Z

Request for data collection review form

1. What questions will you answer with this data?
   What are the enrollment statuses of all recipes a given user is enrolled in when recipes are finished being applied?

2. Why does Mozilla need to answer these questions?
   This data is required for furthering the development of Nimbus. Specifically, it will help data science resolve a current issue where *"we get telemetry from users who do not enroll in deliveries that we expect them to"

ref. telemetry proposal document, linked in the ticket linked on this PR.
1. What alternative methods did you consider to answer these questions? Why were they not sufficient?
  None, no solution is sufficient unless the data collected in this metric is collected.
  We chose to combine what would have been a significant number of new metrics into a single enrollment_status metric.
2. Can current instrumentation answer these questions?
  No
3. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki.

Measurement Description Data Collection Category Tracking Bug #
Enrollment Status Technical data https://mozilla-hub.atlassian.net/browse/EXP-3827

6. Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way.
   The documentation can be found in the proposal document linked in the ticket above (internal only), the [metrics.yaml](https://github.com/mozilla/application-services/pull/5857/files#diff-ca3094e555a194d8c839be789f65c5434e9d04ad84ec363a3072cd89225d9a83) on this PR, and the [Glean dictionary](https://dictionary.telemetry.mozilla.org/).

7. How long will this data be collected?  Choose one of the following:


* I want to permanently monitor this data.
  
  * [[email protected]](mailto:[email protected])
  * [[email protected]](mailto:[email protected])


8. What populations will you measure?
   All populations that use the Nimbus SDK. All of Firefox Android, Firefox iOS, and ultimately all tools using the stateless Nimbus SDK as well.

9. If this data collection is default on, what is the opt-out mechanism for users?
   Nimbus has internal tooling for opting out, it is up to the implementing applications to use this tooling correctly.

10. Please provide a general description of how you will analyze this data.
    We'll analyze it in Jetstream and it will be used to create dashboards for experiment health as well as additional data.

11. Where do you intend to share the results of your analysis?
    They will be shared internally to Mozilla through the Experimenter console as well as ad-hoc through queries against the raw Glean data.

12. Is there a third-party tool (i.e. not Glean or Telemetry) that you are proposing to use for this data collection? If so:
    No

data-review?@eliserichards

Data Review Form (to be filled by Data Stewards)

Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, through this telemetry proposal, metrics.yaml, and the Glean Dictionary.

Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, through the "Send Usage Data" preference in the application settings.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, this data will be permanently collected. Charlie Humphreys ([email protected]) will be monitoring.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1 - technical data.

Is the data collection request for default-on or default-off?

Default on.

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)?

No.

Is the data collection covered by the existing Firefox privacy notice? If unsure: escalate to legal if:

Yes.

Does the data collection use a third-party collection tool? If yes, escalate to legal.

No.

Result

data-review+

yashikakhurana · 2023-10-05T22:12:05Z

@jeddai does this also includes cirrus?

jeddai · 2023-10-05T22:15:56Z

@jeddai does this also includes cirrus?

@yashikakhurana Nope! Cirrus will be implemented in two separate PRs as it'll be a breaking change.

The PRs will be as follows:

Update the CirrusClient object to accept a HostMetricsHandler and call it after enrolling clients
Update Cirrus to actually define a class that implements the HostMetricsHandler interface, pass it to the CirrusClient, have tests for it, and also add the metric to the Cirrus metrics.yaml (the definition will differ slightly).

jhugman

A few nits, a couple of questions, but otherwise looking good!

Of the larger issues, I'm least sure about: the Glean dependency, and exposing testing methods to the FFI.

components/nimbus/tests/test_fs_client.rs

components/nimbus/examples/experiment.rs

jhugman · 2023-10-06T12:42:17Z

components/nimbus/src/enrollment.rs

+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        fmt::Debug::fmt(self, f)
+    }
+}


I'm not sure I like this (forwarding straight to Debug) but I don't see any merit in changing it.

FTR: Display feels like the wrong tool here; since we're using it to Serialize to go over the FFI.

I ended up replacing Display with From<EnumType> for String and manually noting the variant names in a match.

impl From<_> for String also feels weird. I like the explicit match rather than the forwarding to Debug, but Display feels like it's doing the job of Into<String> for &T.

jhugman · 2023-10-06T12:45:11Z

components/nimbus/src/enrollment.rs

+impl EnrollmentStatus {
+    pub fn name(&self) -> String {
+        match self {
+            EnrollmentStatus::Enrolled { .. } => "Enrolled",
+            EnrollmentStatus::NotEnrolled { .. } => "NotEnrolled",
+            EnrollmentStatus::Disqualified { .. } => "Disqualified",
+            EnrollmentStatus::WasEnrolled { .. } => "WasEnrolled",
+            EnrollmentStatus::Error { .. } => "Error",
+        }
+        .into()
+    }
+}


You might be better keeping this as an implementation of fmt::Display to keep it consistent with the other enums— notwithstanding my previous comment about using Display to serialize.

The associated values can still be displayed with Debug.

components/nimbus/src/stateful/nimbus_client.rs

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt

components/nimbus/Cargo.toml

components/nimbus/src/stateful/nimbus_client.rs

jaredlockhart · 2023-10-06T16:22:34Z

Fantastic @jeddai 🎉 🎉 🎉 🎉

components/nimbus/src/tests/stateful/helpers.rs

jhugman

A few more nits and some questions; but nothing that needs another review cycle. Well done, that was a mammoth review/PR cycle.

jhugman · 2023-10-11T16:12:17Z

components/nimbus/src/enrollment.rs

+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        fmt::Debug::fmt(self, f)
+    }
+}


impl From<_> for String also feels weird. I like the explicit match rather than the forwarding to Debug, but Display feels like it's doing the job of Into<String> for &T.

components/nimbus/src/tests/stateful/helpers.rs

components/nimbus/tests/test_fs_client.rs

components/nimbus/examples/experiment.rs

components/nimbus/ios/Nimbus/NimbusCreate.swift

components/nimbus/src/error.rs

components/nimbus/src/metrics.rs

components/nimbus/tests/common/mod.rs

components/nimbus/src/metrics.rs

components/nimbus/src/stateful/nimbus_client.rs

update changelog

jeddai · 2023-10-12T18:41:06Z

Well done, that was a mammoth review/PR cycle.

Thanks! To be fair it's a pretty big change to our existing patterns so I feel like it was fair that it was a longer review

jeddai commented Oct 5, 2023

View reviewed changes

jeddai requested review from jhugman, brennie and eliserichards October 5, 2023 22:01

jeddai changed the title ~~add enrollment_status metric~~ Nimbus: add enrollment_status metric Oct 5, 2023

jeddai added dependencies Pull requests that update a dependency file nimbus Nimbus, the experimentation platform. rust Pull requests that update Rust code labels Oct 5, 2023

jhugman reviewed Oct 6, 2023

View reviewed changes

jeddai commented Oct 6, 2023

View reviewed changes

components/nimbus/src/tests/stateful/helpers.rs Outdated Show resolved Hide resolved

jeddai commented Oct 6, 2023

View reviewed changes

components/nimbus/src/tests/stateful/helpers.rs Outdated Show resolved Hide resolved

jeddai commented Oct 6, 2023

View reviewed changes

components/nimbus/src/tests/stateful/helpers.rs Outdated Show resolved Hide resolved

jeddai requested a review from jhugman October 11, 2023 13:08

jhugman approved these changes Oct 12, 2023

View reviewed changes

jeddai force-pushed the exp-3827-add-enrollment-status-metric branch from d99b9e5 to 9761397 Compare October 12, 2023 17:52

add enrollment_status metric

0a1300c

update changelog

jeddai force-pushed the exp-3827-add-enrollment-status-metric branch from 9761397 to 0a1300c Compare October 12, 2023 18:41

jeddai added this pull request to the merge queue Oct 12, 2023

Merged via the queue into mozilla:main with commit e9e3d51 Oct 12, 2023

jeddai deleted the exp-3827-add-enrollment-status-metric branch October 12, 2023 20:04

jeddai mentioned this pull request Jun 10, 2024

update nimbus metrics.yaml to change notified user and renew an expired metric #6274

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nimbus: add enrollment_status metric #5857

Nimbus: add enrollment_status metric #5857

jeddai commented Oct 5, 2023 •

edited

Loading

codecov-commenter commented Oct 5, 2023 •

edited

Loading

jeddai left a comment

jeddai Oct 5, 2023

jhugman Oct 6, 2023

jeddai Oct 5, 2023 •

edited

Loading

jeddai Oct 5, 2023

jeddai commented Oct 5, 2023

eliserichards commented Oct 5, 2023

Request for data collection review form

yashikakhurana commented Oct 5, 2023

jeddai commented Oct 5, 2023

jhugman left a comment

jhugman Oct 6, 2023

jeddai Oct 6, 2023

jhugman Oct 11, 2023

jhugman Oct 6, 2023

jaredlockhart commented Oct 6, 2023

jhugman left a comment

jhugman Oct 11, 2023

jeddai commented Oct 12, 2023

Nimbus: add enrollment_status metric #5857

Nimbus: add enrollment_status metric #5857

Conversation

jeddai commented Oct 5, 2023 • edited Loading

Pull Request checklist

codecov-commenter commented Oct 5, 2023 • edited Loading

Codecov Report

jeddai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddai Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddai commented Oct 5, 2023

Request for data collection review form

eliserichards commented Oct 5, 2023

Request for data collection review form

Data Review Form (to be filled by Data Stewards)

Result

yashikakhurana commented Oct 5, 2023

jeddai commented Oct 5, 2023

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredlockhart commented Oct 6, 2023

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddai commented Oct 12, 2023

jeddai commented Oct 5, 2023 •

edited

Loading

codecov-commenter commented Oct 5, 2023 •

edited

Loading

jeddai Oct 5, 2023 •

edited

Loading