-
-
Notifications
You must be signed in to change notification settings - Fork 454
Add callback to record the type and amount of data discarded before reaching Sentry #4612
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
Conversation
|
sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java
Outdated
Show resolved
Hide resolved
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
161f873 | 417.88 ms | 448.72 ms | 30.84 ms |
25f281c | 428.48 ms | 463.63 ms | 35.15 ms |
a884d63 | 439.94 ms | 493.38 ms | 53.44 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
161f873 | 1.58 MiB | 2.10 MiB | 530.94 KiB |
25f281c | 1.58 MiB | 2.09 MiB | 522.42 KiB |
a884d63 | 1.58 MiB | 2.10 MiB | 530.95 KiB |
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.
Unless there's a strong reason for it, I would prefer if we moved the DiscardReason
and DataCategory
out of ApiStatus.INTERNAL
, that way we could pass those enums to the callback instead of plain strings.
This probably needs also a refactoring of the intermediate function to take the enums and then convert to string when necessary, but that's a private function so it's fine.
sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java
Outdated
Show resolved
Hide resolved
@@ -124,6 +124,18 @@ private void recordLostEventInternal( | |||
@NotNull String reason, @NotNull String category, @NotNull Long countToAdd) { | |||
final ClientReportKey key = new ClientReportKey(reason, category); | |||
storage.addCount(key, countToAdd); | |||
if (options.getOnDiscard() != null) { | |||
try { | |||
options.getOnDiscard().execute(reason, category, countToAdd); |
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.
There may be duplicate invocations of onDiscard()
after restoreCountsFromClientReport()
is reached. We're discussing whether to change the signature on the callback, so I will address this when we have a decision.
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.
See my comment above
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.
Can we also add a test that proves, we don't double count when restoring counts from a previous ClientReport
envelope item please?
sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java
Outdated
Show resolved
Hide resolved
@@ -124,6 +124,18 @@ private void recordLostEventInternal( | |||
@NotNull String reason, @NotNull String category, @NotNull Long countToAdd) { | |||
final ClientReportKey key = new ClientReportKey(reason, category); | |||
storage.addCount(key, countToAdd); | |||
if (options.getOnDiscard() != null) { | |||
try { | |||
options.getOnDiscard().execute(reason, category, countToAdd); |
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.
See my comment above
sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java
Outdated
Show resolved
Hide resolved
…e signature of onDiscard callback, and avoid duplicate invocation
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.
Nice, looks good to me!
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.
LGTM. Thanks for taking care of this!
📜 Description
Adds a callback field to
SentryOptions
, which is invoked in theClientReportRecorder
when some data is discarded.The callback accepts the discard reason, the type of discarded data, and the number of items discarded as parameters (although the number of items dropped is 1 unless the items are spans). The former two parameters are (currently) strings because the deserializer only returns strings if an envelope containing a
ClientReport
is processed.💡 Motivation and Context
Client reports, introduced by #1982, are currently sent to Sentry. As per the issue below, users migrating from a former Java SDK version have requested a way to hook into the SDK to track when data is discarded before reaching Sentry. They can thus implement custom methods to alert them about issues, for example, with their network configuration.
Closes #3652
💚 How did you test it?
Unit tests, and I ran the console sample.
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps