Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 6 additions & 0 deletions sentry/src/main/java/io/sentry/SentryClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,12 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul
finalizeTransaction(scope, hint);
}

// if event is backfillable or cached we don't need to flush the logs, because it's an event
// from the past. Otherwise we need to flush the logs to ensure they are sent on crash
if (event != null && !isBackfillable && !isCached && event.isCrashed()) {
loggerBatchProcessor.flush(options.getFlushTimeoutMillis());
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure on this.
In theory we could wait for flushTimeoutMillis twice, once here and once to flush the crash.
In practice I expect this method to be instantaneous in Android, as it doesn't wait for them to be sent, and there shouldn't be many logs on Android anyway

Copy link
Member

Choose a reason for hiding this comment

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

IMHO crashes have the highest priority, so I would avoid any extra work/wait which could cause them to be not sent. => Just flush the logs to disk with a minimal timeout.

Copy link
Member Author

Choose a reason for hiding this comment

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

technically, the event is captured first, so it will be sent before the logs anyway
but yeah, let's put a minimal timeout

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the capture of replay and logs after the crash event one.
But now we are waiting only for the crash to be flushed to disk, with the risk of losing replay and logs (except for those 500ms on logs)
Is it fine this way?
@romtsn wdyt?

}

return sentryId;
}

Expand Down
19 changes: 19 additions & 0 deletions sentry/src/test/java/io/sentry/SentryClientTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3023,6 +3023,25 @@ class SentryClientTest {
assertTrue(terminated == true)
}

@Test
fun `flush logs for crash events`() {
val sut = fixture.getSut()
val batchProcessor = mock<ILoggerBatchProcessor>()
sut.injectForField("loggerBatchProcessor", batchProcessor)
sut.captureLog(
SentryLogEvent(SentryId(), SentryNanotimeDate(), "message", SentryLogLevel.WARN),
fixture.scopes.scope,
)

sut.captureEvent(
SentryEvent().apply {
exceptions =
listOf(SentryException().apply { mechanism = Mechanism().apply { isHandled = false } })
}
)
verify(batchProcessor).flush(any())
}

@Test
fun `cleans up replay folder for Backfillable replay events`() {
val dir = File(tmpDir.newFolder().absolutePath)
Expand Down
Loading