-
Notifications
You must be signed in to change notification settings - Fork 27
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
Telemetry #66
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
This was
linked to
issues
Aug 3, 2022
Closed
akornich
approved these changes
Aug 5, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
I left a couple of suggestions to keep in mind during the next big refactoring phase - related to some classes locations/modules.
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.
Description of the change
Behold! Telemetry.
This PR:
The word
Breadcrumb
was chosen as it is the term of art used by Rollbar when describing Telemetry: https://docs.rollbar.com/docs/telemetry.Example has been expanded to show how to "drop breadcrumbs" for telemetry gathering.
Fixes the database not being persisted on Android nor iOS by retrieving the appropriate write directory from each platform:
To imbue any object of persistence capabilities, simply add the
Persistence<?>
mixin to an object, eg:That's all. The object will be given a
TableSet
based on the given record, using a shared sqlite3 database, and configured by theConfig
object as stated byConfigurable
.The lifetime of the persisted records (telemetry, payloads we failed to send) can be configured through
persistenceLifetime
inConfig
.Improved multi-threading to minimize the amount of processing Rollbar does on the main-thread.
The
CoreNotifier
has been generalized into aNotifier
from which two types of asynchronous notifiers can be used,AsyncNotifier
andIsolatedNotifier
, the former uses Dart's asynchronous syntax for single-threaded asynchrony, the latter uses Dart's Isolates for true mutlithreaded asynchrony.IsolatedNotifier
is the default.AsyncNotifier
is used for unit testing, since tests don't work with Isolates. This allows us to fully test the entire library's process flow from the moment we capture an event to the moment we send the occurrence payload to the server.notifier
inConfig
.The minimum required version of Dart and Flutter has been bumped to 3.0.0 to support new features and bring a modern API to users.
The Body was rewritten to better reflect the json structure it represents and that we send to the server. This was also needed for it to hold telemetry.
Equality of our payload data structures is now performed deeply to ensure true equality between instances.
Removed Infrastructure and streamlined the process flow from catching an error to sending it to the server.
Infrastructure
responsibilities were changed accordingly:Notifier
, be itAsyncNotifier
orIsolatedNotifier
.Notifier
is our internal hub, which holds our wrangler, sender and telemetry, and decides who should be called given the event reported by the user-facing Rollbar object.Transformer
and wrangling it to produce a Rollbar-server-friendly payload we can send is now done from theDataWrangler
PersistentHttpSender
.Other changes
Result
since Rollbar successful response is only an UUID, which we can just keep in theResponse
object that used to contain theResult
.null
value for when we have noTransformer
, aNoopTransformer
(no-op) has been created and is now the default. This prevents us from having to do null checks in a few parts of the library, including tests.pana
was reverted back to its latest version, the CI will fail since pana insists on using the currently published0.3.0-beta
rollbar_common
androllbar_dart
to see if the in-development code works , I'll get in touch with pana devs to understand what we're supposed to do here.Type of change
Related issues
Checklists
Development
Code review