Calling Sentry.init and specifying contextTags now has an effect on the Logback SentryAppender #2052
Conversation
sentry-logback/src/main/java/io/sentry/logback/SentryAppender.java
Outdated
Show resolved
Hide resolved
…java Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## 6.x.x #2052 +/- ##
========================================
Coverage ? 80.88%
Complexity ? 3170
========================================
Files ? 228
Lines ? 11717
Branches ? 1574
========================================
Hits ? 9477
Misses ? 1649
Partials ? 591 Continue to review full report at Codecov.
|
|
Missing changelog. |
| @@ -112,8 +116,9 @@ protected void append(@NotNull ILoggingEvent eventObject) { | |||
| CollectionUtils.filterMapEntries( | |||
| loggingEvent.getMDCPropertyMap(), entry -> entry.getValue() != null); | |||
| if (!mdcProperties.isEmpty()) { | |||
| if (!options.getContextTags().isEmpty()) { | |||
| for (final String contextTag : options.getContextTags()) { | |||
| final List<String> contextTags = HubAdapter.getInstance().getOptions().getContextTags(); | |||
There was a problem hiding this comment.
This is a bit confusing, we have the options property, but we read from HubAdapter.getInstance().getOptions().
Should we rather get rid of the options then?
There was a problem hiding this comment.
Yes, that's why i created #2053 I just didn't want to do it now as this quick fix solves the current problem and further improvements didn't seem urgent
| // NOTE: logback.xml properties will not be applied in this case as the SDK has already been | ||
| // initialized |
There was a problem hiding this comment.
I'd rather improve the logging message below instead of adding a comment in the code.
There was a problem hiding this comment.
Hmm looks like I put this in the wrong else path, will update.
| @@ -33,6 +34,7 @@ | |||
| /** Appender for logback in charge of sending the logged events to a Sentry server. */ | |||
| @Open | |||
| public class SentryAppender extends UnsynchronizedAppenderBase<ILoggingEvent> { | |||
| // WARNING: Do not use these options in here, they are only to be used for startup | |||
There was a problem hiding this comment.
I think we should remove this options altogether and set the values directly in the configuration callback when initing the SDK, unless there's a reason to not do so.
There was a problem hiding this comment.
See comment above regarding #2053
There was a problem hiding this comment.
It could be done differently: pass SentryOptions in SentryLogbackInitializer to SentryAppender via constructor - this way options used in both will be in sync - i think it's a better approach than HubAdapter.getInstance.
BUT in case of unhandled exception, event triggered by Logback will be removed by DuplicateEventDetectionEventProcessor. As a result, only event created by SentryExceptionResolver will be send to sentry, and this one does not contain context tags
So unfortunately neither this PR, nor my constructor approach to SentryAppender solve this issue.
📜 Description
No longer used the options property in SentryAppender, instead get Options from HubAdapter.
💡 Motivation and Context
Fix #2043 for Logback
💚 How did you test it?
Demo Code
📝 Checklist
🔮 Next steps