Add Logback integration. #511
Conversation
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
sentry-logback/build.gradle.kts
Outdated
| } | ||
|
|
||
| dependencies { | ||
| implementation(project(":sentry-core")) |
There was a problem hiding this comment.
@marandaneto I am not 100% sure about scope of these dependencies.
There was a problem hiding this comment.
the timber integration has it as an api instead of implementation because one may configure events and breadcrumbs min level (SentryLevel is bundled in sentry-core), so the client needs access to sentry-core classes, if in this case, they dont need to access any of the Sentry classes that are bundled in sentry-core, then implementation is fine
There was a problem hiding this comment.
Wouldn't we document that users using logback can add only this package and get the whole Sentry.X API?
Otherwise users would need to make sure they bump both sentry-core and sentry-logback while keeping them compatible.
I believe it would make sense to have all the transient package's API show up. So api like the timber makes sense?
| implementation(project(":sentry-logback")) | ||
| implementation(project(":sentry-core")) | ||
| implementation(Config.Libs.logbackClassic) | ||
| implementation(Config.Libs.logbackCore) |
There was a problem hiding this comment.
Curious: does classic depend on core or the other way around? I that case we could drop one of them and pin a single dep if transient flows through
|
|
||
| @Override | ||
| public void start() { | ||
| if (dsn != null) { |
There was a problem hiding this comment.
We need some config here to allow a user to opt out for initializing thought the sdk.
It's confusing when the 3 integrations are on and folks don't know what is initing it.
If we can also check if the sdk is already initializing and then warn about it. (Like Sentry.isEnabled) would be best. Last init wins is OK so dispose previous bound hub l/Clientand get it init again ur it's often a mistake.
There was a problem hiding this comment.
Passing config here is optional. If user has at the same time Spring Boot starter.and configures Sentry through it he can omit completely passing options through Logback
There was a problem hiding this comment.
I see. Though there's no way to avoid initializing the SDK via this integration, right?
So if a user adds the dsn to the appender configuration file, it will init the SDK even if it was already initialized.
That's why the suggestion:
If we can also check if the sdk is already initializing and then warn about it. (Like Sentry.isEnabled) would be best. Last init wins is OK so dispose previous bound hub l/Clientand get it init again ur it's often a mistake.
Or at least log a warning with the diagnostic logger
|
BTW super excited to see this landing!! Thanks a lot |
sentry-logback/src/main/java/io/sentry/logback/SentryAppender.java
Outdated
Show resolved
Hide resolved
yep, left a few comments but pretty excited as well, thanks! |
| } | ||
|
|
||
| for (Map.Entry<String, String> entry : loggingEvent.getMDCPropertyMap().entrySet()) { | ||
| event.setTag(entry.getKey(), entry.getValue()); |
There was a problem hiding this comment.
just wondering if we should do setTag(x, y) or setExtras(x, y) or event.setContexts.out(x, y), because getMDCPropertyMap means map diagnostic context, the old version add as a tag so maybe we should keep as it is to keep compatibility
There was a problem hiding this comment.
I think you mean that old version adds as extras? I see that in old SDK there was an option to tell Sentry which MDC tags qualify as tag and which as extra. In 2.0 it's not there anymore so we can either add it or pick tags or extras I think
There was a problem hiding this comment.
yeah its trick: https://github.com/getsentry/sentry-java/blob/master/sentry-logback/src/main/java/io/sentry/logback/SentryAppender.java#L172-L178
I'd suggest then to use setExtras instead of setTag, wdyt?
I don't think we need this configuration to choose when its a tag or not, at least not in the 1st version.
There was a problem hiding this comment.
What's exactly the difference in practice between tags and extras? I see that they are presented differently and users can filter issues by tags. Extras are just metadata attached to an event but users can't do anything with it. Am I correct?
There was a problem hiding this comment.
yes, exactly, and looks like getMDCPropertyMap returns only metadata as well
There was a problem hiding this comment.
But without the ability to search by this metadata its usage is very limited. I imagine users may want to put a userId to MDC and expect to search events for specific user.
Perhaps it makes sense to add a property to SentryAppender mdcTags - similar to get the same functionality as in Sentry SDK 1.x without cluttering sentry-core?
There was a problem hiding this comment.
I get your point, for user for example, our protocol has a dedicated field, but with this integration, it'd be hard to know which key is user-related.
I'd say we can start with everything being an extra and improve from there when we get feedback.
@bruno-garcia opinions?
There was a problem hiding this comment.
Ok I set it as extras 👍
sentry-logback/src/test/kotlin/io/sentry/logback/SentryAppenderTest.kt
Outdated
Show resolved
Hide resolved
sentry-logback/build.gradle.kts
Outdated
| } | ||
|
|
||
| dependencies { | ||
| implementation(project(":sentry-core")) |
There was a problem hiding this comment.
Wouldn't we document that users using logback can add only this package and get the whole Sentry.X API?
Otherwise users would need to make sure they bump both sentry-core and sentry-logback while keeping them compatible.
I believe it would make sense to have all the transient package's API show up. So api like the timber makes sense?
|
|
||
| @Override | ||
| public void start() { | ||
| if (dsn != null) { |
There was a problem hiding this comment.
I see. Though there's no way to avoid initializing the SDK via this integration, right?
So if a user adds the dsn to the appender configuration file, it will init the SDK even if it was already initialized.
That's why the suggestion:
If we can also check if the sdk is already initializing and then warn about it. (Like Sentry.isEnabled) would be best. Last init wins is OK so dispose previous bound hub l/Clientand get it init again ur it's often a mistake.
Or at least log a warning with the diagnostic logger
| * @param loggingEvent the logback event | ||
| * @return the sentry event | ||
| */ | ||
| @SuppressWarnings("JdkObsolete") |
There was a problem hiding this comment.
Why do we need this suppress? Maybe a note will help folks reading the code
| @SuppressWarnings("JdkObsolete") | |
| // Ignoring the obsolete because .... ? | |
| @SuppressWarnings("JdkObsolete") |
There was a problem hiding this comment.
Because we're using java.util.Date and error prone forbids using this class. Ideally we should avoid using this class but I believe we do use it because newer Date classes are not supported on Android.
There was a problem hiding this comment.
Or at least log a warning with the diagnostic logger
We cannot access diagnostic logger from this level as it's a part of SentryOptions. Perhaps we should add this check isSentryEnabled and log warning inside Sentry.init() itself?
There was a problem hiding this comment.
re: Obsolete. That description you gave here would fit well as a comment. So it's clear why we ignored it.
re: logger. Sounds good to at least log on sentry.init!
sentry-logback/src/main/java/io/sentry/logback/SentryAppender.java
Outdated
Show resolved
Hide resolved
sentry-logback/src/test/kotlin/io/sentry/logback/SentryAppenderTest.kt
Outdated
Show resolved
Hide resolved
…Sentry classes when Logback integration is used.
| event.setThrowable(throwableInformation.getThrowable()); | ||
| } | ||
|
|
||
| if (loggingEvent.getThreadName() != null) { |
There was a problem hiding this comment.
what about we add a single thread to the list then? with id, name? When the attachThreads is false that is
📢 Type of change
📜 Description
Added SentryAppender that converts Logback events into Sentry events and sends them to Sentry.
💚 How did you test it?
I added Sample project using Logback, there is also an integration test. On the side I have a POC of Sentry + Logback + Spring Boot integration - https://github.com/maciejwalkowiak/sentry-spring-boot-demo
📝 Checklist