Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Features

- Add an explicit Logs opt-in to the Logback appender ([#5940](https://github.com/getsentry/sentry-java/pull/5940))
- Make `ISpan.startChild` overloads with `SpanOptions` public ([#5927](https://github.com/getsentry/sentry-java/pull/5927))

### Improvements
Expand Down
2 changes: 2 additions & 0 deletions sentry-logback/api/sentry-logback.api
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ public class io/sentry/logback/SentryAppender : ch/qos/logback/core/Unsynchroniz
public fun getMinimumBreadcrumbLevel ()Lch/qos/logback/classic/Level;
public fun getMinimumEventLevel ()Lch/qos/logback/classic/Level;
public fun getMinimumLevel ()Lch/qos/logback/classic/Level;
public fun isEnableLogs ()Z

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be more idiomatic to make this logsEnabled()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik other SDKs are calling the option enableLogs too, so I wanted to align.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, this is the classic tension of consistency vs feeling native to the platform. Given our new goal hierarchy, I would think it should be the one that feels native.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in slack, we should make this feel native.

public fun setEnableLogs (Z)V

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same suggestion as above

Suggested change
public fun setEnableLogs (Z)V
public fun setLogsEnabled (Z)V

public fun setEncoder (Lch/qos/logback/core/encoder/Encoder;)V
public fun setMinimumBreadcrumbLevel (Lch/qos/logback/classic/Level;)V
public fun setMinimumEventLevel (Lch/qos/logback/classic/Level;)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public class SentryAppender extends UnsynchronizedAppenderBase<ILoggingEvent> {
private @NotNull Level minimumBreadcrumbLevel = Level.INFO;
private @NotNull Level minimumEventLevel = Level.ERROR;
private @NotNull Level minimumLevel = Level.INFO;
private boolean enableLogs = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private boolean enableLogs = false;
private boolean logsEnabled = false;

private @Nullable Encoder<ILoggingEvent> encoder;

static {
Expand Down Expand Up @@ -87,7 +88,8 @@ public void start() {

@Override
protected void append(@NotNull ILoggingEvent eventObject) {
if (ScopesAdapter.getInstance().getOptions().getLogs().isEnabled()
if (enableLogs
&& ScopesAdapter.getInstance().getOptions().getLogs().isEnabled()
&& eventObject.getLevel().isGreaterOrEqual(minimumLevel)) {
captureLog(eventObject);
}
Expand Down Expand Up @@ -323,6 +325,14 @@ public void setMinimumLevel(final @Nullable Level minimumLevel) {
return minimumLevel;
}

public void setEnableLogs(final boolean enableLogs) {
this.enableLogs = enableLogs;
}

public boolean isEnableLogs() {
return enableLogs;
}

@ApiStatus.Internal
void setTransportFactory(final @Nullable ITransportFactory transportFactory) {
this.transportFactory = transportFactory;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import kotlin.test.assertTrue
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever
import org.slf4j.Logger
Expand All @@ -55,6 +56,7 @@ class SentryAppenderTest {
encoder: Encoder<ILoggingEvent>? = null,
sendDefaultPii: Boolean = false,
enableLogs: Boolean = false,
enableGlobalLogs: Boolean = enableLogs,
options: SentryOptions = SentryOptions(),
startLater: Boolean = false,
) {
Expand All @@ -71,7 +73,7 @@ class SentryAppenderTest {
this.encoder = encoder
options.dsn = dsn
options.isSendDefaultPii = sendDefaultPii
options.logs.isEnabled = enableLogs
options.logs.isEnabled = enableGlobalLogs
options.logs.loggerBatchProcessorFactory = ILoggerBatchProcessorFactory { options, client ->
LoggerBatchProcessor(options, client, ImmediateExecutorService())
}
Expand All @@ -81,6 +83,7 @@ class SentryAppenderTest {
appender.setMinimumBreadcrumbLevel(minimumBreadcrumbLevel)
appender.setMinimumEventLevel(minimumEventLevel)
appender.setMinimumLevel(minimumLevel)
appender.setEnableLogs(enableLogs)
appender.context = loggerContext
appender.setTransportFactory(transportFactory)
encoder?.context = loggerContext
Expand Down Expand Up @@ -322,6 +325,57 @@ class SentryAppenderTest {
)
}

@Test
fun `does not capture logs by default when aggregate logs are enabled`() {
fixture = Fixture(enableGlobalLogs = true)

assertFalse(fixture.appender.isEnableLogs)
fixture.logger.info("this should not be captured as a log")
Sentry.flush(10)

verify(fixture.transport, never()).send(checkLogs {})
}

@Test
fun `captures logs when local and aggregate logs are enabled`() {
fixture = Fixture(enableLogs = true)

assertTrue(fixture.appender.isEnableLogs)
fixture.logger.info("this should be captured as a log")
Sentry.flush(10)

verify(fixture.transport)
.send(
checkLogs { logs ->
assertEquals("this should be captured as a log", logs.items.first().body)
}
)
}

@Test
fun `captures events and breadcrumbs when local logs are disabled`() {
fixture =
Fixture(
minimumBreadcrumbLevel = Level.INFO,
minimumEventLevel = Level.ERROR,
enableGlobalLogs = true,
)

fixture.logger.info("this should be a breadcrumb")
fixture.logger.error("this should be an event")
Sentry.flush(10)

verify(fixture.transport)
.send(
checkEvent { event ->
assertEquals("this should be an event", event.message?.formatted)
assertEquals("this should be a breadcrumb", event.breadcrumbs?.single()?.message)
},
anyOrNull(),
)
verify(fixture.transport, never()).send(checkLogs {})
}

@Test
fun `converts trace log level to Sentry log level`() {
fixture = Fixture(minimumLevel = Level.TRACE, enableLogs = true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,13 @@
<enabled>true</enabled>
</logs>
</options>
<enableLogs>true</enableLogs>
<!-- Demonstrates how to modify the minimum values -->
<!-- Default for Events is ERROR -->
<minimumEventLevel>WARN</minimumEventLevel>
<!-- Default for Breadcrumbs is INFO -->
<minimumBreadcrumbLevel>DEBUG</minimumBreadcrumbLevel>
<!-- Default for Breadcrumbs is INFO -->
<!-- Default for Logs is INFO -->
<minimumLevel>INFO</minimumLevel>
</appender>

Expand Down
Loading