-
-
Notifications
You must be signed in to change notification settings - Fork 458
Attach MDC properties to logs as attributes #4786
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
4fa3eaf
to
b10a3b4
Compare
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d217708 | 409.83 ms | 474.72 ms | 64.89 ms |
604a261 | 380.65 ms | 451.27 ms | 70.62 ms |
b3d8889 | 420.46 ms | 453.71 ms | 33.26 ms |
c8125f3 | 383.82 ms | 441.66 ms | 57.84 ms |
ee747ae | 396.82 ms | 441.67 ms | 44.86 ms |
3d205d0 | 352.15 ms | 432.53 ms | 80.38 ms |
ce0a49e | 532.00 ms | 609.96 ms | 77.96 ms |
17a0955 | 372.53 ms | 446.70 ms | 74.17 ms |
b3d8889 | 371.69 ms | 432.96 ms | 61.26 ms |
d217708 | 411.22 ms | 430.86 ms | 19.63 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
604a261 | 1.58 MiB | 2.10 MiB | 533.42 KiB |
b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
c8125f3 | 1.58 MiB | 2.10 MiB | 532.32 KiB |
ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
3d205d0 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
ce0a49e | 1.58 MiB | 2.10 MiB | 532.94 KiB |
17a0955 | 1.58 MiB | 2.10 MiB | 533.20 KiB |
b3d8889 | 1.58 MiB | 2.10 MiB | 535.06 KiB |
d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
Previous results on branch: lcian/feat/mdc-logs
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0751889 | 371.21 ms | 442.88 ms | 71.67 ms |
b0d6d12 | 381.21 ms | 410.46 ms | 29.25 ms |
60b38dd | 371.02 ms | 439.54 ms | 68.52 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0751889 | 1.58 MiB | 2.11 MiB | 538.88 KiB |
b0d6d12 | 1.58 MiB | 2.11 MiB | 538.89 KiB |
60b38dd | 1.58 MiB | 2.11 MiB | 538.89 KiB |
I would prefer if we also use As a first step, I'd just add all MDC values according to |
381701f
to
38b07b7
Compare
Yeah that makes sense, changed it. |
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.
Looking good, I've left two comments
* @param properties the properties map (e.g. MDC) | ||
*/ | ||
@ApiStatus.Internal | ||
public static void applyPropertiesToAttributes( |
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.
Opposed to applyPropertiesToEvent
this does not modify properties
, any specific reason why?
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.
For events, we set the properties in targetKeys
as tags on the event, and remove those from the map.
Then, for the properties that are left (i.e. didn't match any of the targetKeys
), we still add them, but as a custom MDC
context.
I might as well move that logic inside this util, as that will make this clearer.
I guess the background reason for why we do things this way is that we have a cost for storing tags on events as they're indexed, so we wanted to minimize the number of tags that we add.
Co-authored-by: Markus Hintersteiner <[email protected]>
} | ||
// put the rest of mdc tags in contexts | ||
if (!contextData.isEmpty()) { | ||
event.getContexts().put("Context Data", contextData); |
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.
Note that with the change this will be reported as MDC
instead of Context Data
. It's not indexed anyways so this shouldn't matter.
📜 Description
options.contextTags
property to filter which properties to attach.beforeSendLog
and server-side scrubbing.LoggerPropertiesUtil
class.property.<key>
ormdc.<key>
) or not. https://sentry.slack.com/archives/C081M1KEQ0L/p1759827935482129💡 Motivation and Context
Close #4783
Close JAVA-197
💚 How did you test it?
Unit tests.
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps