-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #28428: update growth data with new events #28429
Conversation
app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt
Outdated
Show resolved
Hide resolved
*/ | ||
object FirstUriLoadForDay : GrowthData("ja86ek") | ||
object SerpAdClicked : GrowthData("") |
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.
Out of interest, how is the token name generated, and do UriLoaded
and SerpAdClicked
require token names adding?
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.
The token name is generated in the Adjust web app, usually by the marketing/data team. I will wait to land this until those strings have been delivered. Without the tokens, I assume sending the events to the Adjust backend will be no-ops
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.
Gotcha, thanks 👍
app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt
Show resolved
Hide resolved
app/src/test/java/org/mozilla/fenix/components/metrics/DefaultMetricsStorageTest.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.
Just a few nits from my perspective. Maybe useful to have someone with more experience in this area also take a look. My concern is if I've missed any cases where an event should be being handled elsewhere.
6e8a4d8
to
265a500
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.
Thanks for the quick turnaround, this looks good to me.
@Mergifyio backport releases_v109.0.0 |
✅ Backports have been created
|
(cherry picked from commit 7d84c53) # Conflicts: # app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt # app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt # app/src/main/java/org/mozilla/fenix/telemetry/TelemetryMiddleware.kt # app/src/main/java/org/mozilla/fenix/utils/Settings.kt # app/src/main/res/values/preference_keys.xml # app/src/test/java/org/mozilla/fenix/components/metrics/DefaultMetricsStorageTest.kt # app/src/test/java/org/mozilla/fenix/telemetry/TelemetryMiddlewareTest.kt
(cherry picked from commit 7d84c53)
…mobile/fenix#28429 (mozilla-mobile/fenix#28750) (cherry picked from commit 11227fa) Co-authored-by: MatthewTighe <matthewdtighe@gmail.com>
Decided to address all the new requirements in a single issue instead of splitting them, in order to make the uplift a bit more manageable. In retrospect, probably would've been good to do separate commits but I wasn't sure how well mergify would handle that so thanks in advance!
Note: we will need to wait to land this until it can be updated with actual event tokens for the new growth data but as of posting they had not been generated yet. Thought it would be more beneficial to get reviews early
Pull Request checklist
QA
To download an APK when reviewing a PR (after all CI tasks finished running):
Checks
at the top of the PR page.firefoxci-taskcluster
group on the left to expand all tasks.build-debug
task.View task in Taskcluster
in the newDETAILS
section.GitHub Automation
Fixes #28428