Add callback to record the type and amount of data discarded before reaching Sentry #4612
Conversation
alexander-alderman-webb
requested review from
adinauer,
lcian,
markushi,
romtsn and
stefanosiano
as code owners
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
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
| storage.addCount(key, countToAdd); | ||
| if (options.getOnDiscard() != null) { | ||
| try { | ||
| options.getOnDiscard().execute(reason, category, countToAdd); |
There was a problem hiding this comment.
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.
adinauer
left a comment
adinauer
left a comment
There was a problem hiding this comment.
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
| storage.addCount(key, countToAdd); | ||
| if (options.getOnDiscard() != null) { | ||
| try { | ||
| options.getOnDiscard().execute(reason, category, countToAdd); |
sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java
Outdated
Show resolved
Hide resolved
…e signature of onDiscard callback, and avoid duplicate invocation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
adinauer
left a comment
adinauer
left a comment
There was a problem hiding this comment.
LGTM. Thanks for taking care of this!
📜 Description
Adds a callback field to
SentryOptions, which is invoked in theClientReportRecorderwhen 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
ClientReportis 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
sendDefaultPIIis enabled.🔮 Next steps