Bug 1728128 - Ensure events database is initialized at a time when metrics can be recorded #692
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
badboy
approved these changes
Sep 2, 2021
| // | ||
| // This is fine, we are inside a dispatched task that is guaranteed to run before any | ||
| // other task. No external API call will be executed before we leave this task. | ||
| Context.initialized = true; |
Member
There was a problem hiding this comment.
Is anything else depending on this value and could now cause problems because there's still intialization happening after this value is set to true?
Contributor
Author
There was a problem hiding this comment.
Only ping submission / upload check if Glean is initialized, but both actions usually happen inside dispatched tasks (aside from onUploadDisabled, when we send the deletion-request ping undispatched). Because the initialize task is always executed before of all other dispatched tasks so we should be fine.
This is a good exercise though. We need to really think hard before adding anything to this initialize function. It's very of fragile, order matters, etc.
There is no reason not to do it and as soon as these properties are set they can be used, so better to set them as soon as possible. This was not causing any bugs at the moment, but might in the future.
If we had such an error message, we might not have missed the issue about not being able to record the glean.restarted event upon initialization. The whole problem there was trying to access Context.uploadEnabled before it was set.
This is important in order to make sure test mimick correctly what happens in real usage.
I know the solution here is a bit sloppy, but I decided to pick my battles and got he easy route on this one.
brizental
force-pushed
the
1728128-restarted-webext-qt-verify
branch
from
e7764e1 to
302fe31
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After tests, I confirmed none of the sample apps were recording the
glean.restartedevent when Glean was initialized :/ That was the case, because we were callingeventsDatabase.initializebefore settingContext.uploadEnabled. That meantContext.uploadEnabledwasundefinedupon events database initialization andundefinedbeing a falsy value,.shouldRecord()was returningfalse, which resulted in theglean.restartedandexecution_countermetrics not being recorded.In 8324569 I fix that and in 4cb6118 I add a test to ensure that if this behaviour ever changes we will see test failures.
The commits in between these two commits are there to make sure the test does fail if the behaviour is changed. I actually found a bug in the way we were reseting Glean in tests. We were not reseting all the preoperties in the
Context, only a few. That would prevent the test I added from actually failing when it had to.Pull Request checklist
glean/folder, run:npm run testRuns all testsnpm run lintRuns all lintersCHANGELOG.mdor an explanation of why it does not need oneDocumentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work