Skip to content

Feat: Add client reports#1982

Merged
adinauer merged 29 commits into6.x.xfrom
feat/client-reports
Apr 21, 2022
Merged

Feat: Add client reports#1982
adinauer merged 29 commits into6.x.xfrom
feat/client-reports

Conversation

@adinauer
Copy link
Copy Markdown
Member

@adinauer adinauer commented Apr 7, 2022 •
edited
Loading

📜 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

adinauer
adinauer commented Apr 7, 2022
View reviewed changes
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 7, 2022 •
edited
Loading

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.

adinauer
adinauer commented Apr 7, 2022
View reviewed changes
adinauer
adinauer commented Apr 7, 2022
View reviewed changes
adinauer
adinauer commented Apr 7, 2022
View reviewed changes
marandaneto
marandaneto reviewed Apr 8, 2022
View reviewed changes
marandaneto
marandaneto reviewed Apr 8, 2022
View reviewed changes
marandaneto
marandaneto reviewed Apr 8, 2022
View reviewed changes
marandaneto
marandaneto reviewed Apr 8, 2022
View reviewed changes
marandaneto
marandaneto reviewed Apr 8, 2022
View reviewed changes
marandaneto
marandaneto reviewed Apr 8, 2022
View reviewed changes
marandaneto
marandaneto reviewed Apr 8, 2022
View reviewed changes
philipphofmann
philipphofmann reviewed Apr 11, 2022
View reviewed changes
Copy link
Copy Markdown
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 👏

denrase
denrase reviewed Apr 13, 2022
View reviewed changes
denrase
denrase reviewed Apr 13, 2022
View reviewed changes
denrase
denrase reviewed Apr 13, 2022
View reviewed changes
adinauer
adinauer commented Apr 14, 2022
View reviewed changes
philipphofmann
philipphofmann reviewed Apr 14, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

marandaneto commented Apr 19, 2022

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?

@denrase denrase mentioned this pull request Apr 21, 2022
Merged
13 tasks
@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
philipphofmann
philipphofmann approved these changes Apr 21, 2022
View reviewed changes
Copy link
Copy Markdown
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
@alexander-alderman-webb alexander-alderman-webb mentioned this pull request Aug 7, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@denrase denrase denrase left review comments

@philipphofmann philipphofmann philipphofmann approved these changes

@mattjohnsonpint mattjohnsonpint Awaiting requested review from mattjohnsonpint

@romtsn romtsn Awaiting requested review from romtsn

+2 more reviewers

@maciejwalkowiak maciejwalkowiak maciejwalkowiak left review comments

@marandaneto marandaneto marandaneto approved these changes

Reviewers whose approvals may not affect merge requirements

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

@adinauer @codecov-commenter @marandaneto @maciejwalkowiak @philipphofmann @denrase