Fix setting context tags on events captured by Spring #2060
Conversation
| @Override | ||
| public @NotNull SentryEvent process( | ||
| @NotNull SentryEvent event, @Nullable Map<String, Object> hint) { | ||
| final Map<String, String> contextMap = MDC.getCopyOfContextMap(); |
There was a problem hiding this comment.
Perhaps we could filter only to events with mechanism type HandlerExceptionResolver, otherwise for events send from logback this logic will be applied twice.
On the other hand, if we don't filter by mechanism type, context tags are also applied to events triggered manually with Sentry.captureException().
There was a problem hiding this comment.
Is it bad if direct captures also get the MDC context added to the tags?
There was a problem hiding this comment.
You can also filter by event.getLogger since it sets event.setLogger(record.getLoggerName()); for JUL and Logback, but IMO, we should add MDC anyways, if they don't want, they won't add in the options.getContextTags() in the first place.
There was a problem hiding this comment.
So if i am getting it right, we are keeping it as it is, correct?
There was a problem hiding this comment.
Yes, but that's just my opinion, I know that we should always be skeptical about adding a lot of tags since high cardinality data is more expensive, so please double-check with @bruno-garcia
There was a problem hiding this comment.
I'd keep it this way to add MDC tags regardless of logging framework.
The way it is implemented it'll just override existing tags and should just work or de we expect any problems regarding MDC being thread local?
I think we also need PRs #2052 , #2054 and #2057 in case we're not dealing with spring.
There was a problem hiding this comment.
If someone does not use Spring Boot Starter, then they have to configure Logback explicitly in logback.xml - meaning they have to provide configuration - so it's somewhat expected that context tags set in logback.xml are taken into account. Then I believe we do not need any other change.
There was a problem hiding this comment.
As discussed today with @maciejwalkowiak we do want #2052, #2054 and #2057 in order to support Sentry.init calls.
| * Attaches context tags defined in {@link SentryOptions#getContextTags()} from {@link MDC} to | ||
| * {@link SentryEvent#getTags()}. | ||
| */ | ||
| public final class ContextTagsEventProcessor implements EventProcessor { |
There was a problem hiding this comment.
Should this event processor live in the sentry-spring module?
There was a problem hiding this comment.
It put it to sentry-spring-boot-starter because it needs slf4j and it is provided only in that module. If we decide to move it somewhere else and add there sl4fj perhaps it could be sentry module as there is nothing really Spring specific here.
|
This PR fixes #2043 but only if you're using the |
Yes, as @maciejwalkowiak mentioned above we expect devs to use |
|
Opened #2065 to move it into the spring module. |
Codecov Report
@@ Coverage Diff @@
## 6.x.x #2060 +/- ##
========================================
Coverage ? 81.07%
Complexity ? 3217
========================================
Files ? 230
Lines ? 11819
Branches ? 1572
========================================
Hits ? 9582
Misses ? 1650
Partials ? 587 Continue to review full report at Codecov.
|
📜 Description
Fixes setting context tags on events triggered by
SentryExceptionResolver.💡 Motivation and Context
Fixes #2043
💚 How did you test it?
📝 Checklist