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

Regenerate the client id / first run date if needed #2723

Merged
merged 2 commits into from Apr 12, 2019

Conversation

Dexterp37
Copy link
Contributor

Old builds of products using glean, which were already executed once, once updated, might not correctly report the client id and the first run date. Since these are likely low number of developer builds, we simply regenerate these fields.

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

Old builds of products using glean, which were already
executed once, once updated, might not correctly report
the client id and the first run date. Since these are
likely low number of developer builds, we simply regenerate
these fields.
@Dexterp37 Dexterp37 requested a review from a team as a code owner April 12, 2019 12:36
@codecov
Copy link

codecov bot commented Apr 12, 2019

Codecov Report

Merging #2723 into master will increase coverage by 0.16%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2723      +/-   ##
============================================
+ Coverage     82.69%   82.86%   +0.16%     
- Complexity     2885     2911      +26     
============================================
  Files           359      365       +6     
  Lines         12260    12403     +143     
  Branches       1797     1810      +13     
============================================
+ Hits          10139    10278     +139     
- Misses         1379     1382       +3     
- Partials        742      743       +1
Impacted Files Coverage Δ Complexity Δ
...ain/java/mozilla/components/service/glean/Glean.kt 94.23% <90%> (+1.37%) 1 <0> (ø) ⬇️
...omponents/service/glean/private/EventMetricType.kt 85.29% <0%> (-0.92%) 16% <0%> (ø)
...ents/service/glean/storages/EventsStorageEngine.kt 84.21% <0%> (-0.88%) 25% <0%> (ø)
...components/service/glean/private/UuidMetricType.kt 91.66% <0%> (-0.34%) 15% <0%> (-1%)
...ponents/feature/tabs/toolbar/TabsToolbarFeature.kt 100% <0%> (ø) 2% <0%> (?)
...la/components/feature/tabs/tabstray/TabsFeature.kt 100% <0%> (ø) 6% <0%> (?)
...ponents/feature/tabs/tabstray/TabsTrayPresenter.kt 97.56% <0%> (ø) 11% <0%> (?)
...onents/feature/tabs/tabstray/TabsTrayInteractor.kt 100% <0%> (ø) 5% <0%> (?)
...ts/feature/tabs/toolbar/TabCounterToolbarButton.kt 90.47% <0%> (ø) 2% <0%> (?)
...va/mozilla/components/feature/tabs/TabsUseCases.kt 100% <0%> (ø) 1% <0%> (?)

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 227c932...09933e7. Read the comment docs.

Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

r+ (Do we have/should we file) a bug to look at this again in 6 months to remove the cruft (after it's hopefully no longer needed)?

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. My comment is more just thinking out loud, and not a requirement to merge this.

// vs using the full class name with the namespace).
// This should be mostly devs and super early adopters, so just regenerate
// the data.
UuidsStorageEngine.record(GleanInternalMetrics.clientId, UUID.randomUUID())
Copy link
Contributor

Choose a reason for hiding this comment

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

Random thought -- we could record an error on the metric here so we could see how often this happens. If it shows up again in the future, it might help us detect mistakes...

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, I like this idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that might be a good idea. The thing is that, in contrast to desktop, we have no alerting on this for now. It it would be up to humans (that's us!) to manually check in that for now. I'd rather have a monitor or alerting on schema validation errors (that caught this!!!!), which is a lower hanging fruit with the tooling that's available right now. Additionally, I'm planning on prioritizing the integration test.

@Dexterp37
Copy link
Contributor Author

r+ (Do we have/should we file) a bug to look at this again in 6 months to remove the cruft (after it's hopefully no longer needed)?

I filed this https://bugzilla.mozilla.org/show_bug.cgi?id=1543979

@Dexterp37 Dexterp37 merged commit 7b83e92 into mozilla-mobile:master Apr 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants