Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

1570647 - Do not send 'metrics' ping if the app was not used #3993

Merged
merged 1 commit into from
Aug 5, 2019
Merged

1570647 - Do not send 'metrics' ping if the app was not used #3993

merged 1 commit into from
Aug 5, 2019

Conversation

travis79
Copy link
Member

@travis79 travis79 commented Aug 1, 2019

This attempts to address the issue around sending of metrics pings when the app hasn't been used for that day.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.

@travis79 travis79 requested a review from a team as a code owner August 1, 2019 20:36
@travis79
Copy link
Member Author

travis79 commented Aug 1, 2019

This is silly but the test I added (startupCheck happens when returning from background when Glean is already initialized) runs perfect in debug but fails in release, so any ideas on why that might be would be greatly appreciated.

@travis79 travis79 force-pushed the mps-fix branch 2 times, most recently from 981ae04 to e7fc7b9 Compare August 1, 2019 21:57
mdboom
mdboom approved these changes Aug 1, 2019
View reviewed changes
Copy link
Contributor

@mdboom mdboom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Just one additional thing to test, if possible.

...ice/glean/src/main/java/mozilla/components/service/glean/scheduler/GleanLifecycleObserver.kt Outdated
// If this is the first ON_START event since the app was launched, Glean wouldn't be
// initialized yet.
if (Glean.isInitialized()) {
Glean.metricsPingScheduler.startupCheck()
Copy link
Contributor

Choose a reason for hiding this comment

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

The risk of this change might be that when it enters foreground multiple times during the same 24 hour period, the ping is scheduled for sending twice (at the same 4AM instant the next morning). I think the fact that we use WorkManager.getInstance().enqueueUniqueWork() makes that impossible, but I wonder if there's a way to write a test for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could also check to see if a Worker is already enqueued, which I didn't really think to do either. I agree we should be okay since we do use the enqueueUniqueWork but a test and/or a check wouldn't hurt.

Copy link
Contributor

Choose a reason for hiding this comment

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

One test could be to use the workmanager testing api to enqueue two unique work units and check if both are there. But that would only test the WorkManager API, and I bet that such a test is already there, for them

...rvice/glean/src/main/java/mozilla/components/service/glean/scheduler/MetricsPingScheduler.kt Outdated Show resolved Hide resolved
Dexterp37
Dexterp37 suggested changes Aug 2, 2019
View reviewed changes
...ice/glean/src/main/java/mozilla/components/service/glean/scheduler/GleanLifecycleObserver.kt Outdated Show resolved Hide resolved
...ice/glean/src/main/java/mozilla/components/service/glean/scheduler/GleanLifecycleObserver.kt Outdated
// If this is the first ON_START event since the app was launched, Glean wouldn't be
// initialized yet.
if (Glean.isInitialized()) {
Glean.metricsPingScheduler.startupCheck()
Copy link
Contributor

Choose a reason for hiding this comment

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

One test could be to use the workmanager testing api to enqueue two unique work units and check if both are there. But that would only test the WorkManager API, and I bet that such a test is already there, for them

...e/glean/src/test/java/mozilla/components/service/glean/scheduler/MetricsPingSchedulerTest.kt Outdated Show resolved Hide resolved
...e/glean/src/test/java/mozilla/components/service/glean/scheduler/MetricsPingSchedulerTest.kt Outdated Show resolved Hide resolved
...rvice/glean/src/main/java/mozilla/components/service/glean/scheduler/MetricsPingScheduler.kt Outdated Show resolved Hide resolved
@travis79 travis79 force-pushed the mps-fix branch 2 times, most recently from e47bf4d to af0d5ce Compare August 2, 2019 18:23
@travis79 travis79 force-pushed the mps-fix branch 4 times, most recently from 68bc886 to 50556c4 Compare August 5, 2019 15:25
@travis79 travis79 requested a review from Dexterp37 August 5, 2019 15:40
Dexterp37
Dexterp37 approved these changes Aug 5, 2019
View reviewed changes
Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

This looks good to me. When porting this to glean-core, we should document the new specifications (we don't send if we were not in foreground in the measurement window).

Let's get a review from Sebastian and Mike as well. Please also take this for a manual test drive.

@Dexterp37 Dexterp37 requested review from mdboom and pocmo August 5, 2019 15:46
@travis79
Copy link
Member Author

travis79 commented Aug 5, 2019

This looks good to me. When porting this to glean-core, we should document the new specifications (we don't send if we were not in foreground in the measurement window).

Let's get a review from Sebastian and Mike as well. Please also take this for a manual test drive.

Sounds good to me, working on the manual test drive today and looks like you already flagged @mdboom and @pocmo

mdboom
mdboom approved these changes Aug 5, 2019
View reviewed changes
Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

LGTM

@travis79 travis79 force-pushed the mps-fix branch 3 times, most recently from c235333 to a51d300 Compare August 5, 2019 20:07
@travis79
1570647 - Do not send 'metrics' ping if the app was not used
0492a92 
Also removes unnecessary type arguments from MetricsPingSchedulerTest.kt
@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #3993 into master will decrease coverage by 2.49%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #3993     +/-   ##
===========================================
- Coverage     82.55%   80.05%   -2.5%     
+ Complexity     3660     3360    -300     
===========================================
  Files           481      450     -31     
  Lines         15686    14826    -860     
  Branches       2347     2221    -126     
===========================================
- Hits          12949    11869   -1080     
- Misses         1758     2038    +280     
+ Partials        979      919     -60
Impacted Files Coverage Δ Complexity Δ
...ponents/service/experiments/ExperimentEvaluator.kt 85.57% <0%> (-0.97%) 57% <0%> (-1%)
...ts/service/glean/storages/CountersStorageEngine.kt
...s/service/glean/storages/DatetimesStorageEngine.kt
...omponents/service/glean/private/EventMetricType.kt
...ts/service/glean/storages/BooleansStorageEngine.kt
...s/service/glean/storages/TimespansStorageEngine.kt
...vice/glean/private/TimingDistributionMetricType.kt
...service/glean/storages/ExperimentsStorageEngine.kt
...illa/components/browser/storage/sync/Connection.kt
...components/service/glean/private/UuidMetricType.kt
... and 52 more

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 a3e5bc2...0492a92. Read the comment docs.

@travis79 travis79 merged commit 54a0a58 into mozilla-mobile:master Aug 5, 2019
@travis79 travis79 deleted the mps-fix branch August 5, 2019 21:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mdboom mdboom mdboom approved these changes

@Dexterp37 Dexterp37 Dexterp37 approved these changes

@pocmo pocmo Awaiting requested review from pocmo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@travis79 @mdboom @pocmo @Dexterp37