-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Calling Sentry.init and specifying contextTags now has an effect on the Log4j SentryAppender #2054
Conversation
sentry-log4j2/src/main/java/io/sentry/log4j2/SentryAppender.java
Outdated
Show resolved
Hide resolved
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.
brrrr, this feels wrong again, having to maintain 2 different properties for the same purpose 🤣
Would it be possible to set contextTags
directly on the hub
in the SentryAppender constructor? This way we could eliminate the field contextTags
and wouldn't need to set it in start
.
Although this could be a problem if the appender was initialized but never started, we still set the contextTags on the hub, don't know if that can be ever the case
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
I've added something to #2053 , should we deal with this at a later time? |
Yeah sure, was just a quick suggestion, but we can do it later |
Codecov Report
@@ Coverage Diff @@
## 6.x.x #2054 +/- ##
========================================
Coverage ? 81.07%
Complexity ? 3217
========================================
Files ? 230
Lines ? 11821
Branches ? 1572
========================================
Hits ? 9584
Misses ? 1650
Partials ? 587 Continue to review full report at Codecov.
|
I like the suggestion but I'm not sure about the consequences so I don't wanna rush it. The quick fix should suffice for now I hope. |
sentry-log4j2/src/main/java/io/sentry/log4j2/SentryAppender.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
I believe it should be put on hold until #2052 (review) is resolved, so that we have ideally one consistent approach between logging frameworks. |
📜 Description
No longer used the
contextTags
property inSentryAppender
, instead get via Options from HubAdapter.💡 Motivation and Context
Fix #2043 for Log4j
💚 How did you test it?
Demo code
📝 Checklist
🔮 Next steps