-
-
Notifications
You must be signed in to change notification settings - Fork 257
Feat: Record & Send Client Reports #813
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
Feat: Record & Send Client Reports #813
Conversation
…-report # Conflicts: # dart/lib/src/client_reports/client_report_recorder.dart
Codecov Report
@@ Coverage Diff @@
## feat/client-report-envelope-item #813 +/- ##
====================================================================
- Coverage 92.27% 89.55% -2.72%
====================================================================
Files 26 102 +76
Lines 777 3132 +2355
====================================================================
+ Hits 717 2805 +2088
- Misses 60 327 +267
Continue to review full report at Codecov.
|
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.
Nothing to add yet. I'm looking forward to more code 😀
…-report # Conflicts: # dart/lib/src/client_reports/outcome.dart
@@ -19,19 +22,22 @@ class HttpTransport implements Transport { | |||
|
|||
final RateLimiter _rateLimiter; | |||
|
|||
final ClientReportRecorder _clientReportRecorder; |
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.
Maybe ClientReportRecorder
should be a property of SentryOptions
? cc @philipphofmann
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.
We don't put dependencies / objects to the options in ObjC. I guess you should follow your SDKs conventions and you can answer best how to follow these I think.
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.
We don't really have a central object/place to init dependencies AFAIK, but we rather worked with singletons for now, or other objects construct their dependencies internally, like in this PR.
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.
With the refactoring to not expose the new classes I moved the instance of ClientReportRecorder
to options. It is mainly used to access the instance in Hub
, SentryClient
and tests.
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.
This looks pretty good 👍 . I maybe found two issues.
…-report # Conflicts: # dart/test/client_reports/client_report_test.dart
#skip-changelog
📜 Description
Integrate recorder into
Transport
implementations.Record dropped events & transactions.
Exposes
void recordLostEvent(DiscardReason reason, DataCategory category)
onSentryClient
andHub
.💡 Motivation and Context
Record client reports.
💚 How did you test it?
Unit tests.
📝 Checklist
🔮 Next steps