Skip to content
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: Add client reports #1982

Merged
merged 29 commits into from
Apr 21, 2022
Merged

Feat: Add client reports #1982

merged 29 commits into from
Apr 21, 2022

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Apr 7, 2022

📜 Description

Adds recording discarded events and sending them with client reports to
Sentry. Discarded events give insight into where the SDK drops certain
events like, for example, beforeSend or rate limiting.

💡 Motivation and Context

Fixes #1894

💚 How did you test it?

Unit tests, Emulator, Some local load testing to ensure counts are correct when hit from multiple threads

📝 Checklist

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

🔮 Next steps

@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2022

Codecov Report

Merging #1982 (2403eeb) into 6.x.x (be0ebe0) will increase coverage by 0.07%.
The diff coverage is 75.12%.

@@             Coverage Diff              @@
##              6.x.x    #1982      +/-   ##
============================================
+ Coverage     80.73%   80.81%   +0.07%     
- Complexity     3051     3125      +74     
============================================
  Files           220      228       +8     
  Lines         11266    11597     +331     
  Branches       1506     1551      +45     
============================================
+ Hits           9096     9372     +276     
- Misses         1603     1646      +43     
- Partials        567      579      +12     
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/SentryOptions.java 81.17% <33.33%> (-1.25%) ⬇️
.../sentry/clientreport/NoOpClientReportRecorder.java 60.00% <60.00%> (ø)
...main/java/io/sentry/clientreport/ClientReport.java 62.00% <62.00%> (ø)
...in/java/io/sentry/clientreport/DiscardedEvent.java 63.15% <63.15%> (ø)
sentry/src/main/java/io/sentry/SentryItemType.java 86.66% <66.66%> (+1.48%) ⬆️
...n/java/io/sentry/clientreport/ClientReportKey.java 66.66% <66.66%> (ø)
...ry/transport/apache/ApacheHttpClientTransport.java 71.87% <68.85%> (+2.54%) ⬆️
...a/io/sentry/clientreport/ClientReportRecorder.java 74.32% <74.32%> (ø)
...ry/src/main/java/io/sentry/SentryEnvelopeItem.java 86.71% <81.25%> (-0.69%) ⬇️
...src/main/java/io/sentry/transport/RateLimiter.java 85.85% <83.33%> (+9.22%) ⬆️
... and 18 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 be0ebe0...2403eeb. Read the comment docs.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Retryables happen on startup where counts are 0 so no need. Also it would have to prevent double restoring from persisted envelopes that retry multiple times.
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.

This is a high-level review of the code. I didn't look at any tests or too close to the code. Overall this PR looks great. Thanks for doing this @adinauer 👏

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.

Nothing to add.

@marandaneto
Copy link
Contributor

If DataCategory is used outside of the transport layer, we can move out to the root sentry folder.

It's ok having a clientreport package and public classes as long as they are marked as internal with annotations.

Classes that are not used outside of the client report can be package-private.

Public methods that are in public classes and need to be used in different packages, can also be marked as internal with annotations, e.g. Options#getClientReportRecorder.

@bruno-garcia opinions?

@adinauer adinauer marked this pull request as ready for review April 21, 2022 08:49
@adinauer adinauer requested a review from romtsn as a code owner April 21, 2022 08:49
@marandaneto marandaneto changed the title Feat/client reports Feat: Add client reports Apr 21, 2022
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.

Didn't see any issues from a high-level perspective. Thanks for doing this 💪

@adinauer adinauer merged commit 72f1911 into 6.x.x Apr 21, 2022
@adinauer adinauer deleted the feat/client-reports branch April 21, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants