Skip to content
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

Apply OkHttp listener auto finish timestamp to all running spans #3167

Merged
merged 3 commits into from
Jan 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -14,6 +14,7 @@

### Fixes

- Apply OkHttp listener auto finish timestamp to all running spans ([#3167](https://github.com/getsentry/sentry-java/pull/3167))
- Fix not eligible for auto proxying warnings ([#3154](https://github.com/getsentry/sentry-java/pull/3154))
- Set default fingerprint for ANRv2 events to correctly group background and foreground ANRs ([#3164](https://github.com/getsentry/sentry-java/pull/3164))
- This will improve grouping of ANRs that have similar stacktraces but differ in background vs foreground state. Only affects newly-ingested ANR events with `mechanism:AppExitInfo`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import io.sentry.ISpan
import io.sentry.SentryDate
import io.sentry.SentryLevel
import io.sentry.SpanDataConvention
import io.sentry.SpanStatus
import io.sentry.TypeCheckHint
import io.sentry.okhttp.SentryOkHttpEventListener.Companion.CONNECTION_EVENT
import io.sentry.okhttp.SentryOkHttpEventListener.Companion.CONNECT_EVENT
Expand Down Expand Up @@ -158,10 +157,12 @@ internal class SentryOkHttpEvent(private val hub: IHub, private val request: Req

// We forcefully finish all spans, even if they should already have been finished through finishSpan()
eventSpans.values.filter { !it.isFinished }.forEach {
// If a status was set on the span, we use that, otherwise we set its status as error.
it.status = it.status ?: SpanStatus.INTERNAL_ERROR
moveThrowableToRootSpan(it)
it.finish()
if (finishDate != null) {
it.finish(it.status, finishDate)
markushi marked this conversation as resolved.
Show resolved Hide resolved
} else {
it.finish()
}
}
beforeFinish?.invoke(callRootSpan)
// We report the client error here, after all sub-spans finished, so that it will be bound
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import io.sentry.okhttp.SentryOkHttpEventListener.Companion.REQUEST_HEADERS_EVEN
import io.sentry.okhttp.SentryOkHttpEventListener.Companion.RESPONSE_BODY_EVENT
import io.sentry.okhttp.SentryOkHttpEventListener.Companion.RESPONSE_HEADERS_EVENT
import io.sentry.okhttp.SentryOkHttpEventListener.Companion.SECURE_CONNECT_EVENT
import io.sentry.test.ImmediateExecutorService
import io.sentry.test.getProperty
import okhttp3.Protocol
import okhttp3.Request
Expand All @@ -31,17 +32,14 @@ import okhttp3.mockwebserver.MockWebServer
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.argThat
import org.mockito.kotlin.argumentCaptor
import org.mockito.kotlin.check
import org.mockito.kotlin.eq
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
import org.mockito.kotlin.spy
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever
import java.util.concurrent.Future
import java.util.concurrent.RejectedExecutionException
import kotlin.RuntimeException
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFalse
Expand Down Expand Up @@ -254,16 +252,6 @@ class SentryOkHttpEventTest {
)
}

@Test
fun `when finishEvent, all running spans are finished with internal_error status`() {
val sut = fixture.getSut()
sut.startSpan("span")
val spans = sut.getEventSpans()
assertNull(spans["span"]!!.status)
sut.finishEvent()
assertEquals(SpanStatus.INTERNAL_ERROR, spans["span"]!!.status)
}

@Test
fun `when finishEvent, does not override running spans status if set`() {
val sut = fixture.getSut()
Expand All @@ -273,6 +261,7 @@ class SentryOkHttpEventTest {
spans["span"]!!.status = SpanStatus.OK
assertEquals(SpanStatus.OK, spans["span"]!!.status)
sut.finishEvent()
assertTrue(spans["span"]!!.isFinished)
assertEquals(SpanStatus.OK, spans["span"]!!.status)
}

Expand Down Expand Up @@ -533,18 +522,15 @@ class SentryOkHttpEventTest {
}

@Test
fun `scheduleFinish schedules finishEvent`() {
val mockExecutor = mock<ISentryExecutorService>()
val captor = argumentCaptor<Runnable>()
whenever(mockExecutor.schedule(captor.capture(), any())).then {
captor.lastValue.run()
mock<Future<Runnable>>()
}
fixture.hub.options.executorService = mockExecutor
fun `scheduleFinish schedules finishEvent and finish running spans to specific timestamp`() {
fixture.hub.options.executorService = ImmediateExecutorService()
val sut = spy(fixture.getSut())
val timestamp = mock<SentryDate>()
sut.startSpan(CONNECTION_EVENT)
sut.scheduleFinish(timestamp)
verify(sut).finishEvent(eq(timestamp), anyOrNull())
val spans = sut.getEventSpans()
assertEquals(timestamp, spans[CONNECTION_EVENT]?.finishDate)
}

@Test
Expand Down
Loading