-
-
Notifications
You must be signed in to change notification settings - Fork 461
Report discarded log bytes #4871
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
base: main
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Report discarded log bytes ([#4871](https://github.com/getsentry/sentry-java/pull/4871))If none of the above apply, you can opt out of this check by adding |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2124a46 | 319.19 ms | 415.04 ms | 95.85 ms |
| b3d8889 | 420.46 ms | 453.71 ms | 33.26 ms |
| d217708 | 409.83 ms | 474.72 ms | 64.89 ms |
| 3d205d0 | 352.15 ms | 432.53 ms | 80.38 ms |
| fcec2f2 | 357.47 ms | 447.32 ms | 89.85 ms |
| 889ecea | 367.58 ms | 437.52 ms | 69.94 ms |
| 1df7eb6 | 397.04 ms | 429.64 ms | 32.60 ms |
| ce0a49e | 532.00 ms | 609.96 ms | 77.96 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| b750b96 | 408.98 ms | 480.32 ms | 71.34 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2124a46 | 1.58 MiB | 2.12 MiB | 551.51 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 3d205d0 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| 889ecea | 1.58 MiB | 2.11 MiB | 539.75 KiB |
| 1df7eb6 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| ce0a49e | 1.58 MiB | 2.10 MiB | 532.94 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| b750b96 | 1.58 MiB | 2.10 MiB | 533.19 KiB |
stefanosiano
left a comment
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.
Looks good for the most part, but reading the doc i understood we shouldn't count json characters in the byte size, which we are doing here
Perhaps a simple method in the log object that counts each key and value bytes is a good alternative?
|
We're currently discussing, whether this simplified count as if it were serialized is OK or if we want to replicate what relay is doing. |
alexander-alderman-webb
left a comment
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.
Looks good!
To record: no scientific byte count needed, as users are not billed on logs that are dropped in the SDK. So while the logic differs slightly from relay, counting the bytes in the serialized JSON emitted by the SDK seems like the most pragmatic choice and good enough for this use case.
| * @param serializer the serializer | ||
| * @param logger the logger | ||
| * @param serializable the serializable object | ||
| * @return the size in bytes, or -1 if serialization fails |
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.
The function never returns -1, let's update the docstring
| final @NotNull long logEventNumberOfBytes = | ||
| JsonSerializationUtils.byteSizeOf( | ||
| options.getSerializer(), options.getLogger(), tmpLogEvent); | ||
| options | ||
| .getClientReportRecorder() | ||
| .recordLostEvent( | ||
| DiscardReason.BEFORE_SEND, DataCategory.LogByte, logEventNumberOfBytes); |
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.
not blocking: when dropping elsewhere we do not record a lost bytes when there's a serialization failure, whereas here we record a lost log_bytes event with 0 bytes.
I think this is okay as is, since we shouldn't encounter serialization failures anyway; we're also not strict about accurate byte reporting.
📜 Description
Report discarded log size in bytes as part of client reports.
💡 Motivation and Context
Since log count is not shown to customers in UI, we want to report log size too.
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps