Add a breadcrumb automatically for changes in network connectivity #448
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
robinmacharg
suggested changes
Jan 27, 2020
There was a problem hiding this comment.
Looks good and makes sense. A few inline comments, mostly asking for - implicit or explicit - clarification, and a couple of general queries:
- There's no CI testing for actual connectivity changes; is that something we could look into?
- And it would be useful to know the anticipated impact on normal event reporting. For instance would rapid repeated connectivity changes cause other event types to be dropped? If connectivity is unstable there may not be time to send existing stored events before the next connectivity change (which may in turn may cause an app failure).
- The choice of Bugsnag as a connectivity endpoint had no fallback and so assumes 100% uptime. Since we cache events we don't actually require 100% uptime for event reporting, but we do for connectivity breadcrumbs. Are there more "reliable" optional alternates that could be used as fallback endpoints?
kattrali
force-pushed
the
kattrali/breadcrumb-connectivity
branch
2 times, most recently
from
b5c8a7f
to
df651af
Compare
This question might have been a misunderstanding - this changeset just leaves breadcrumbs for connectivity changes, no events are being generated by the event. |
kattrali
force-pushed
the
kattrali/breadcrumb-connectivity
branch
from
df651af
to
597e79a
Compare
fractalwrench
requested review from
fractalwrench
and removed request for
robinmacharg
fractalwrench
approved these changes
Feb 12, 2020
…iate * Allows to callee to respond to events where the connectivity was lost * Removes need to keep a reference to a Connectivity object, since it masked the fact that it required global state to function and There Can Only Be One. * Peppers more documentation throughout
kattrali
force-pushed
the
kattrali/breadcrumb-connectivity
branch
from
597e79a
to
9aedd59
Compare
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.
Adds a breadcrumb each time the connection status noticeably changes, such as between no connectivity, cellular, and wifi, using the Bugsnag server URL as a proxy for overall connectivity.
Refactors the connectivity checker as a part of this change to report connectivity in a way suitable to append to reports, reduce object references and increase documentation and testing.