Add Log4j2 integration. #537
Conversation
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
sentry-log4j2/src/main/java/io/sentry/log4j2/SentryAppender.java
Outdated
Show resolved
Hide resolved
sentry-log4j2/src/main/java/io/sentry/log4j2/SentryAppender.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @ApiStatus.Internal | ||
| void setTransport(@Nullable ITransport transport) { |
There was a problem hiding this comment.
Dow e really need this or can we use the client here instead?
I assume it's for testing.
We could take a IHub in the constructor instead.
An internal overload would take it. The public overload calls into it with HubAdapter.getInstance for the prod use case
There was a problem hiding this comment.
yeah I agree with @bruno-garcia
Sentry.init already overwrites the NoOpTransport to HttpTransport on Sdk init.
if it's for the sake of testing, I'd use the hub, I guess the other integrations also have references for transport and I'd refactor as well unless there's a need that I'm not aware of.
There was a problem hiding this comment.
The problem is that some properties are set in MainEventProcessor - like sdk version. This is why I moved to mocking transport instead of Hub - some things are just not possible to test when just Hub is mocked.
There was a problem hiding this comment.
I see, makes sense.
whats about creating a new Hub, passing options, but mocking the transport in the options, it'd be pretty much the same, but the entry point here would be the Hub still as every other integration.
There was a problem hiding this comment.
I have nothing against but I also don't see how adding this one level of indirection makes anything better..?
There was a problem hiding this comment.
we could discuss what would make more sense here, but that's how it is in all the others tests, we never make a reference directly to the transport, but rather the entry point, and mock things in the options if necessary, so we could assert and verify directly from them.
if we decide that this would make more sense (assert on the transport), I'd rather change everywhere, I just would like to avoid a mix of different ways of doing things.
maybe @bruno-garcia has opinions.
There was a problem hiding this comment.
Some information like SDK version is added later, in MainEventProcessor - this is why i need to mock transport on lower level.
I started with mocking Hub, but this does not give me tools to test integration properly.
| private @NotNull Level minimumBreadcrumbLevel = Level.INFO; | ||
| private @NotNull Level minimumEventLevel = Level.ERROR; |
There was a problem hiding this comment.
does it make sense to add this either to options or to the Level class as static default fields and we just read the default values from them? cus I see these 2 values repeated in all the integrations, like:
SentryOptions.DEFAULT_BREADCRUMB_LEVEL
SentryOptions.DEFAULT_EVENT_LEVEL
or something like that.
There was a problem hiding this comment.
It depends if we want to pollute sentry-core with integration specific code. At this stage core is not aware of the whole mechanism of attaching breadcrumbs to events in logging integrations.
There was a problem hiding this comment.
I'd be fine with that, to not DRY, of course, it depends on a case by case, but this duplication is already in 4 integrations.
It'd be something similar, defining default values to be reused elsewhere.
There was a problem hiding this comment.
Ok understood. This would mean that we need to add conversion from SentryLevel to logging-framework-level - as these levels in appenders are are from log4j2/Logback packages.
There was a problem hiding this comment.
yeah the converters for sure, it's just to reuse the default levels, nothing more.
There was a problem hiding this comment.
IMHO I also rather to keep them out since core has nothing to do with min breadcrumb level of a logger integration
DRY doesn't mean avoid any and all "duplication" by breaking cohesion
sentry-log4j2/src/main/java/io/sentry/log4j2/SentryAppender.java
Outdated
Show resolved
Hide resolved
sentry-log4j2/src/test/kotlin/io/sentry/log4j2/SentryAppenderTest.kt
Outdated
Show resolved
Hide resolved
sentry-log4j2/src/test/kotlin/io/sentry/log4j2/SentryAppenderTest.kt
Outdated
Show resolved
Hide resolved
sentry-samples/sentry-samples-log4j2/src/main/java/io/sentry/samples/log4j2/Main.java
Show resolved
Hide resolved
|
@bruno-garcia @marandaneto please take another look. I would be happy if we could finish it today. |
3 participants
📢 Type of change
📜 Description
Log4j2 integration based on existing Logback integration.
💡 Motivation and Context
Feature parity with 1.x
📝 Checklist
🔮 Next steps