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

Send a baseline at startup if the previous session ended abruptly #687

Merged
merged 10 commits into from
Feb 11, 2020

Conversation

Dexterp37
Copy link
Contributor

@Dexterp37 Dexterp37 commented Feb 5, 2020

This attempts to address the problem of 'force-close' interfering with the baseline ping. It implements the following logic:

In addition to the current behaviour:

  • when the Glean SDK is initialized, it checks for a dirty flag:
  • when going to background, clear the dirty flag after computing the foreground duration and submitting the baseline ping.

This will guarantee that whenever the product using the Glean SDK is force-closed or crashes before it has the chance to send a baseline ping with a specific reason, signaling that there was a dirty flag set.

Important: there will be no foreground duration reported with the baseline ping with reason “dirty-startup”.

TODO

  • Add test coverage in Kotlin
  • Add a changelog entry

@Dexterp37 Dexterp37 marked this pull request as ready for review February 5, 2020 16:15
@auto-assign auto-assign bot requested a review from badboy February 5, 2020 16:15
@Dexterp37 Dexterp37 requested a review from mdboom February 5, 2020 16:15
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Small nits but overall this is nearly good to go when the reason codes PR lands.
Please flag me again after it's rebased on that.

docs/user/pings/baseline.md Show resolved Hide resolved
glean-core/ffi/src/lib.rs Outdated Show resolved Hide resolved
glean-core/pings.yaml Outdated Show resolved Hide resolved
glean-core/src/lib.rs Outdated Show resolved Hide resolved
glean-core/src/lib.rs Show resolved Hide resolved
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.

Just small nits -- optional to address them.

INTERNAL_STORAGE,
&dirty_bit_metric.meta().identifier(self),
) {
Some(Metric::Boolean(b)) => b,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use unwrap_or(false) instead of this match clause here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By changing to that, we get a expected bool, found enum metrics::Metric`` - leaving this as it is, we can always fixup later!

@Dexterp37 Dexterp37 merged commit da1e014 into mozilla:master Feb 11, 2020
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.

3 participants