Closes #19147: Move startup metrics to right after Glean initialization #19252

rocketsroger · 2021-04-26T19:27:11Z

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

gabrielluong · 2021-04-26T23:26:16Z

Is there also a data eng/sci peer that can take a look at this since we are also updating the glean types for some metrics?

rocketsroger · 2021-04-27T14:06:36Z

Is there also a data eng/sci peer that can take a look at this since we are also updating the glean types for some metrics?

Sure, I'll add reviewer from data science.

rocketsroger · 2021-04-27T14:14:10Z

@Dexterp37 please confirm that this change is what we still want for #12131. Thanks,

badboy

Blocking here while I wait for confirmation that just changing the type won't break anything.

This is now code that's executed earlier, is there any way to verify that it has a neglibile impact on startup?

app/src/main/java/org/mozilla/fenix/FenixApplication.kt

badboy · 2021-04-27T15:52:37Z

app/src/main/java/org/mozilla/fenix/FenixApplication.kt

+     * limitations of the metrics ping.  Events are only sent in a metrics ping if the user have made
+     * changes between each ping.  However, in some cases we want current values to be sent even if


Not sure which "Events" you mean here. Can you expand?
As I see it the shared preferences are simply there to store preferences, making it the source of truth, so reading from them is the right thing to do. What am I missing?

Oh I see now that is just copied from the old code.
Still wonder what this part means.

Yes, this was copied from the original function. I will update it so it makes more sense. Thanks,

badboy · 2021-04-27T16:00:18Z

app/metrics.yaml

@@ -1270,7 +1270,7 @@ metrics:

 preferences:
  show_search_suggestions:
-    type: string_list
+    type: boolean


Just for your awareness: This will work, but land in a different column.
Old: metrics.string_list.preferences_show_search_suggestions
New: metrics.boolean.preferences_show_search_suggestions

That means comparing these across versions is a bit more involved, but not impossible.
(There should never be a client that has both set though)

(I'm still waiting for confirmation that what I said is actually true)

Yes, please confirm that this is OK. Since this workaround and because currently there are high number of startup metric pings that have null values, I would argue that comparing against data we've collected before is not very useful.

Yeah, I agree it sounds like it's worthwhile to make the change. You still may want to add in the description field "Prior to release X, this metric was of type string_list" so that if someone wanted to trace back through the (problematic) historical data they'd at least know how...

This kind of change is not supported by our tooling, a similar change of a string to a datetime in the past has caused delays on our schema deploys: mozilla/mozilla-schema-generator#118

Type changes is typically frowned upon at a low level because it breaks schema evolution rules that allows for low operational overhead during ingestion time. It's possible to support changing types in the schema generator based on the behavior of moving the probe to a new namespace (which avoids a schema incompatible change). However, this will be confusing from a metadata perspective since we lose context on the type change.

I suggest expiring these probes and creating a new probe with the correct type, while clearly documenting the reason for deprecation so that tools like the glean dictionary can pick this up.

Understood. I will look into expiring the probes and create new ones with the correct type. Thanks!

@rocketsroger Can you ping me when you think you need a final review? Thanks!

@gabrielluong Sure, this should be the final change but I'll ping you once I have the go ahead from the data science team. Thanks,

gabrielluong · 2021-04-27T01:50:16Z

app/src/main/java/org/mozilla/fenix/FenixApplication.kt

+     * This function is called right after Glean is initialized.  Part of this function depends on
+     * shared preferences to be updated so the correct value is sent with the metrics ping.
+     *
+     * The reason we're using shared preferences to track some of these values is due to the
+     * limitations of the metrics ping.  Events are only sent in a metrics ping if the user have made
+     * changes between each ping.  However, in some cases we want current values to be sent even if
+     * the user have not changed anything between pings.


I don't see why we gotta keep the double spaces when it's not utilized everywhere.

Suggested change

* This function is called right after Glean is initialized. Part of this function depends on

* shared preferences to be updated so the correct value is sent with the metrics ping.

*

* The reason we're using shared preferences to track some of these values is due to the

* limitations of the metrics ping. Events are only sent in a metrics ping if the user have made

* changes between each ping. However, in some cases we want current values to be sent even if

* the user have not changed anything between pings.

* This function is called right after Glean is initialized. Part of this function depends on

* shared preferences to be updated so the correct value is sent with the metrics ping.

*

* The reason we're using shared preferences to track some of these values is due to the

* limitations of the metrics ping. Events are only sent in a metrics ping if the user have made

* changes between each ping. However, in some cases we want current values to be sent even if

* the user have not changed anything between pings.

Sure, I'll update this since I'll rewrite this. Not sure why originally this was double spaced.

rocketsroger · 2021-04-27T16:48:15Z

This is now code that's executed earlier, is there any way to verify that it has a neglibile impact on startup?

Yes, I think this will have some impact on startup performance. However, this is currently the only workaround suggestion from the Glean team. Once the Glean investigation is over we will update this if there is a better solution.

gabrielluong · 2021-04-27T20:25:32Z

Will take another look at this thoroughly tomorrow.

gabrielluong · 2021-04-28T22:15:38Z

Sorry, didn't get to this today, will review it tomorrow!

codecov-commenter · 2021-04-29T18:48:53Z

Codecov Report

Merging #19252 (9d95275) into master (5b5e7eb) will increase coverage by 0.01%.
The diff coverage is 57.57%.

❗ Current head 9d95275 differs from pull request most recent head c545a7b. Consider uploading reports for the commit c545a7b to get more accurate results

@@             Coverage Diff              @@
##             master   #19252      +/-   ##
============================================
+ Coverage     34.87%   34.88%   +0.01%     
+ Complexity     1618     1617       -1     
============================================
  Files           540      540              
  Lines         21816    21806      -10     
  Branches       3249     3249              
============================================
- Hits           7609     7608       -1     
+ Misses        13307    13300       -7     
+ Partials        900      898       -2

Impacted Files	Coverage Δ	Complexity Δ
...la/fenix/components/metrics/GleanMetricsService.kt	`19.88% <ø> (-5.91%)`	`2.00 <0.00> (-3.00)`
...rc/main/java/org/mozilla/fenix/FenixApplication.kt	`26.20% <57.14%> (+16.00%)`	`10.00 <2.00> (+2.00)`
...ain/java/org/mozilla/fenix/components/Analytics.kt	`66.66% <100.00%> (ø)`	`1.00 <0.00> (ø)`
...rg/mozilla/fenix/settings/account/AccountUiView.kt	`36.58% <0.00%> (+2.43%)`	`4.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b5e7eb...c545a7b. Read the comment docs.

badboy

Looks good to me now!

gabrielluong

Do we need to check with the perf-team?

gabrielluong · 2021-05-03T20:49:57Z

app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt

@@ -856,10 +846,7 @@ private val Event.wrapper: EventWrapper<*>?
    }

 class GleanMetricsService(


Could use a kDoc if we can come up with a nice description of what GleanMetricsService does

Sure, I'll try to describe this service.

gabrielluong · 2021-05-04T18:53:36Z

app/src/main/java/org/mozilla/fenix/FenixApplication.kt

+    private fun setPreferenceMetrics(
+        settings: Settings
+    ) {
+        // We purposefully make all of our preferences the string_list format to make data analysis


I imagine we should change this comment if not remove it entirely since they're boolean now. I think this comment shouldn't be applied to the with block either since it only applies to line 613-623.

Good catch. Thanks I'll update

rocketsroger · 2021-05-05T14:11:43Z

Do we need to check with the perf-team?

I'll notify the perf-team. This will affect the startup performance but unfortunately there's no other solutions.

…n initialization

See: mozilla-mobile/fenix#19252

rocketsroger requested review from a team as code owners April 26, 2021 19:27

rocketsroger added the needs:review PRs that need to be reviewed label Apr 26, 2021

gabrielluong self-requested a review April 27, 2021 01:52

rocketsroger requested a review from Dexterp37 April 27, 2021 14:12

rocketsroger added the needs:data-science label Apr 27, 2021

mdboom requested a review from badboy April 27, 2021 14:20

badboy suggested changes Apr 27, 2021

View reviewed changes

gabrielluong reviewed Apr 27, 2021

View reviewed changes

rocketsroger force-pushed the FB_19147 branch from a71c9c2 to 22701b2 Compare April 27, 2021 17:18

rocketsroger requested review from badboy and gabrielluong April 27, 2021 17:19

rocketsroger closed this Apr 28, 2021

rocketsroger reopened this Apr 28, 2021

rocketsroger force-pushed the FB_19147 branch from 22701b2 to 9d95275 Compare April 29, 2021 18:28

rocketsroger requested a review from mdboom April 29, 2021 18:29

rocketsroger closed this Apr 29, 2021

rocketsroger reopened this Apr 29, 2021

rocketsroger closed this Apr 30, 2021

rocketsroger reopened this Apr 30, 2021

badboy approved these changes May 3, 2021

View reviewed changes

rocketsroger removed the needs:data-science label May 3, 2021

gabrielluong approved these changes May 4, 2021

View reviewed changes

gabrielluong assigned badboy, mdboom and gabrielluong May 4, 2021

Closes mozilla-mobile#19147: Move startup metrics to right after Glea…

c545a7b

…n initialization

rocketsroger force-pushed the FB_19147 branch from 9d95275 to c545a7b Compare May 5, 2021 14:41

rocketsroger removed the needs:review PRs that need to be reviewed label May 5, 2021

rocketsroger merged commit 74047cd into mozilla-mobile:master May 5, 2021

rocketsroger deleted the FB_19147 branch May 5, 2021 15:11

wlach mentioned this pull request May 6, 2021

Add annotations for changed Glean startup metrics mozilla/glean-annotations#37

Closed

mdboom mentioned this pull request May 7, 2021

Issue #19147: Move set startup metrics off main thread #19397

Merged

3 tasks

wlach added a commit to mozilla/glean-annotations that referenced this pull request Jun 11, 2021

Add annotations for replaced Fenix metrics

bc0cc76

See: mozilla-mobile/fenix#19252

wlach mentioned this pull request Jun 11, 2021

Add annotations for replaced Fenix metrics (fixes #37) mozilla/glean-annotations#43

Merged

wlach added a commit to mozilla/glean-annotations that referenced this pull request Jun 16, 2021

Add annotations for replaced Fenix metrics (fixes #37) (#43)

d4cb7ac

See: mozilla-mobile/fenix#19252

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #19147: Move startup metrics to right after Glean initialization #19252

Closes #19147: Move startup metrics to right after Glean initialization #19252

rocketsroger commented Apr 26, 2021

gabrielluong commented Apr 26, 2021

rocketsroger commented Apr 27, 2021

rocketsroger commented Apr 27, 2021

badboy left a comment

badboy Apr 27, 2021

badboy Apr 27, 2021

rocketsroger Apr 27, 2021

badboy Apr 27, 2021

rocketsroger Apr 27, 2021 •

edited

mdboom Apr 28, 2021

acmiyaguchi Apr 28, 2021

rocketsroger Apr 29, 2021

gabrielluong Apr 29, 2021

rocketsroger Apr 29, 2021

gabrielluong Apr 27, 2021

rocketsroger Apr 27, 2021

rocketsroger commented Apr 27, 2021

gabrielluong commented Apr 27, 2021

gabrielluong commented Apr 28, 2021

codecov-commenter commented Apr 29, 2021 •

edited

badboy left a comment

gabrielluong left a comment

gabrielluong May 3, 2021

rocketsroger May 5, 2021

gabrielluong May 4, 2021

rocketsroger May 5, 2021

rocketsroger commented May 5, 2021

		* limitations of the metrics ping. Events are only sent in a metrics ping if the user have made
		* changes between each ping. However, in some cases we want current values to be sent even if

		@@ -856,10 +846,7 @@ private val Event.wrapper: EventWrapper<*>?
		}

		class GleanMetricsService(

Closes #19147: Move startup metrics to right after Glean initialization #19252

Closes #19147: Move startup metrics to right after Glean initialization #19252

Conversation

rocketsroger commented Apr 26, 2021

Pull Request checklist

To download an APK when reviewing a PR:

gabrielluong commented Apr 26, 2021

rocketsroger commented Apr 27, 2021

rocketsroger commented Apr 27, 2021

badboy 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

rocketsroger Apr 27, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocketsroger commented Apr 27, 2021

gabrielluong commented Apr 27, 2021

gabrielluong commented Apr 28, 2021

codecov-commenter commented Apr 29, 2021 • edited

Codecov Report

badboy left a comment

Choose a reason for hiding this comment

gabrielluong 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

rocketsroger commented May 5, 2021

rocketsroger Apr 27, 2021 •

edited

codecov-commenter commented Apr 29, 2021 •

edited