Bug 1704606 - Rust: Don't return a result from submit_ping
#1613
Conversation
This file contains 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
Dexterp37
approved these changes
May 4, 2021
|
(of course with CI passing) |
badboy
force-pushed
the
1704606-no-more-result-bool
branch
from
c0ad933 to
ca96c58
Compare
badboy
force-pushed
the
1704606-no-more-result-bool
branch
from
ca96c58 to
cdba52c
Compare
chutten
approved these changes
May 4, 2021
| if let Err(err) = self.internal_pings.events.submit(self, Some("inactive")) { | ||
| log::warn!("Failed to submit events ping on inactive: {}", err); | ||
| if !self.internal_pings.events.submit(self, Some("inactive")) { | ||
| log::info!("events ping not submitted on inactive"); |
There was a problem hiding this comment.
I wonder if it should still be louder than info!, though, since it should be impossible. Thoughts? Maybe it needs instrumentation/error stream.
There was a problem hiding this comment.
Not having an events ping on inactive is normal behavior though, if no events have been recorded nothing will be submitted.
badboy
force-pushed
the
1704606-no-more-result-bool
branch
from
c54697e to
e2378d6
Compare
The `Result<bool>` value returned previously was not expressing what's really happening anymore. It was impossible to encounter an `Err` variant after recent changes. Technically this is a breaking change for glean-core, but that library is not consumed by any outsiders.
Unfortunately there's a left-over "DO NOT COMMIT" in there. Fix is pending upstream: mozilla/glean_parser#322
Submitting can't hard-fail anymore, so we can reduce our warning to info here.
This is required because of the changes made to ping.submit before and how it was used internally. Previously in event_database, when triggering startup event pings, we only checked if storing a ping did not fail. We did not check if any ping was actually submitted. So the code around it launched a task on the workmanager, which then subsequently checked and did nothing. The test ensured that, and triggering the work manager was just fine because there _was_ a pending task. With the changes we now return `true` if any ping was submitted. Or `false` if no event ping was submitted on startup. Now on startup no task was triggered and thus no workmanager can be triggered. That made the test fail because `triggerWorkManager` requires a task to be enqueued. We can force a task by also recording into the builtin "events" ping. It's a small hack, but allows us to test that late-registered pings with events are correctly skipped AND their events are deleted.
badboy
force-pushed
the
1704606-no-more-result-bool
branch
from
e2378d6 to
e652433
Compare
Codecov Report
@@ Coverage Diff @@
## main #1613 +/- ##
=======================================
Coverage 30.30% 30.30%
=======================================
Files 1 1
Lines 33 33
=======================================
Hits 10 10
Misses 23 23 Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The
Result<bool>value returned previously was not expressing what'sreally happening anymore.
It was impossible to encounter an
Errvariant after recent changes.Technically this is a breaking change for glean-core, but that library
is not consumed by any outsiders.