Skip to content

Feat: Client Report Recorder #809

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 13, 2022
Merged

Feat: Client Report Recorder #809

merged 8 commits into from
Apr 13, 2022

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Mar 28, 2022 •
edited
Loading

#skip-changelog

📜 Description

  • Introduce model files for client reports.
  • Introduce class for "recording" lost events.

💡 Motivation and Context

This is the first PR for implementing client reports. It's based on the work done in the JS SDK, except that some of the business logic from transport is encapsulated in a separate class.

💚 How did you test it?

Unit tests.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

  • This PR will be merged on a separate branch, so we can split up development for the whole feature.
  • Integrate the recorder into transport, create envelopes, send reports, record events in SDK.

@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2022 •
edited
Loading

Codecov Report

Merging #809 (93c8cea) into feat/client-reports (636d851) will decrease coverage by 0.66%.
The diff coverage is 77.77%.

@@                   Coverage Diff                   @@
##           feat/client-reports     #809      +/-   ##
=======================================================
- Coverage                89.83%   89.17%   -0.67%     
=======================================================
  Files                      114       92      -22     
  Lines                     3670     2918     -752     
=======================================================
- Hits                      3297     2602     -695     
+ Misses                     373      316      -57
Impacted Files Coverage Δ
dart/lib/src/transport/rate_limit.dart 100.00% <ø> (ø)
dart/lib/src/client_reports/outcome.dart 25.00% <25.00%> (ø)
dart/lib/src/transport/data_category.dart 50.00% <50.00%> (ø)
dart/lib/src/client_reports/client_report.dart 100.00% <100.00%> (ø)
...lib/src/client_reports/client_report_recorder.dart 100.00% <100.00%> (ø)
dart/lib/src/client_reports/discarded_event.dart 100.00% <100.00%> (ø)
dart/lib/src/transport/rate_limit_parser.dart 95.83% <100.00%> (-0.17%) ⬇️
dart/lib/src/transport/rate_limiter.dart 87.80% <100.00%> (ø)
flutter/lib/src/flutter_sentry_attachment.dart
.../lib/src/navigation/sentry_navigator_observer.dart
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 636d851...93c8cea. Read the comment docs.

@denrase denrase marked this pull request as ready for review March 28, 2022 13:26
marandaneto
marandaneto reviewed Mar 29, 2022
View reviewed changes
marandaneto
marandaneto reviewed Mar 29, 2022
View reviewed changes
dart/lib/src/client_reports/client_report_recorder.dart Outdated
final DateTime Function() _dateTimeProvider;
final Map<_QuantityKey, int> _quantities = {};

void recordLostEvent(final Outcome reason, final RateLimitCategory category) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to double check if RateLimitCategory is accurate and if it's missing new entries.
See https://github.com/getsentry/develop/pull/535/files and https://github.com/getsentry/develop/pull/536/files

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, hard to say. We are missing the internal type in RateLimitCategory, but have the __all__ type, which i think is specific to the rate limiting implementation and should probably never be used here. Should we create a separate enum type to be on the safe side?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait those 2 PRs get reviewed/merged, we can rename to DataCategory at least in case this is not a breaking change to public APIs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also renamed RateLimitCategory to DataCategory on Cocoa as relay uses DataCategory for both rate limits and client reports.

Copy link
Collaborator Author

@denrase denrase Apr 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to the appropriate PR/commit? Intrested in how you are handling the "all" case, assuming that it should not be valid in the client report context and is specific to rate limiting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering, as the Dart implementation was ported from sentry-java and we have an all category there: https://github.com/getsentry/sentry-java/blob/b45944c93d53f68bc1e64f5ac4c9a5400b252d44/sentry/src/main/java/io/sentry/transport/RateLimiter.java#L25

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't added any special validation code to prevent usage of DataCategory enum values not meant for client reports as we don't need to take care of this (just asked).

marandaneto
marandaneto reviewed Mar 29, 2022
View reviewed changes
philipphofmann
philipphofmann reviewed Mar 30, 2022
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor conceptual comments. Thanks, @denrase for taking care of client reports 🚀

dart/lib/src/client_reports/client_report_recorder.dart Outdated
final DateTime Function() _dateTimeProvider;
final Map<_QuantityKey, int> _quantities = {};

void recordLostEvent(final Outcome reason, final RateLimitCategory category) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also renamed RateLimitCategory to DataCategory on Cocoa as relay uses DataCategory for both rate limits and client reports.

@denrase denrase requested a review from marandaneto April 5, 2022 08:13
brustolin
brustolin approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@denrase denrase merged commit 893a689 into feat/client-reports Apr 13, 2022
@denrase denrase deleted the feat/client-report-recorder branch April 13, 2022 07:28
@denrase denrase mentioned this pull request Apr 13, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipphofmann philipphofmann philipphofmann left review comments

@marandaneto marandaneto marandaneto approved these changes

@brustolin brustolin brustolin approved these changes

@mattjohnsonpint mattjohnsonpint Awaiting requested review from mattjohnsonpint

@adinauer adinauer Awaiting requested review from adinauer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@denrase @codecov-commenter @philipphofmann @adinauer @marandaneto @brustolin