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

Support Spring's new RestClient with auto configuration (#3198) #3199

Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication;
import org.springframework.boot.autoconfigure.web.client.RestClientAutoConfiguration;
import org.springframework.boot.autoconfigure.web.client.RestTemplateAutoConfiguration;
import org.springframework.boot.autoconfigure.web.reactive.function.client.WebClientAutoConfiguration;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
Expand All @@ -65,6 +66,7 @@
import org.springframework.graphql.execution.DataFetcherExceptionResolverAdapter;
import org.springframework.scheduling.quartz.SchedulerFactoryBean;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.web.client.RestClient;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.reactive.function.client.WebClient;
import org.springframework.web.servlet.HandlerExceptionResolver;
Expand Down Expand Up @@ -357,6 +359,17 @@ public SentrySpanRestTemplateCustomizer sentrySpanRestTemplateCustomizer(IHub hu
}
}

@Configuration(proxyBeanMethods = false)
@AutoConfigureBefore(RestClientAutoConfiguration.class)
@ConditionalOnClass(RestClient.class)
@Open
static class SentrySpanRestClientConfiguration {
@Bean
public SentrySpanRestClientCustomizer sentrySpanRestClientCustomizer(IHub hub) {
return new SentrySpanRestClientCustomizer(hub);
}
}

@Configuration(proxyBeanMethods = false)
@AutoConfigureBefore(WebClientAutoConfiguration.class)
@ConditionalOnClass(WebClient.class)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package io.sentry.spring.boot.jakarta;

import com.jakewharton.nopen.annotation.Open;
import io.sentry.IHub;
import io.sentry.spring.jakarta.tracing.SentrySpanClientHttpRequestInterceptor;
import org.jetbrains.annotations.NotNull;
import org.springframework.boot.web.client.RestClientCustomizer;
import org.springframework.web.client.RestClient;

@Open
class SentrySpanRestClientCustomizer implements RestClientCustomizer {
private final @NotNull SentrySpanClientHttpRequestInterceptor interceptor;

public SentrySpanRestClientCustomizer(final @NotNull IHub hub) {
this.interceptor = new SentrySpanClientHttpRequestInterceptor(hub);
}

@Override
public void customize(final @NotNull RestClient.Builder restClientBuilder) {
restClientBuilder.requestInterceptors(clientHttpRequestInterceptors -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a tricky one, I saw the same concept in the SentrySpanRestTemplateCustomizer but I'm not sure if the contains can catch that a SentrySpanClientHttpRequestInterceptor is already registered or not, as it is being instantiated in the constructor, so different instances can be registered easily. We might need a proper equals in the SentrySpanClientHttpRequestInterceptor

Copy link
Member

Choose a reason for hiding this comment

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

So far I'm not aware of any reported problems about this in SentrySpanRestTemplateCustomizer so I'd say we can do the same thing here and improve it if problems come up.

// As the SentrySpanClientHttpRequestInterceptor is being created in this class, this might not work
// if somebody registers it from an outside.
if (!clientHttpRequestInterceptors.contains(interceptor)) {
clientHttpRequestInterceptors.add(interceptor);
}
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import org.springframework.core.Ordered
import org.springframework.core.annotation.Order
import org.springframework.scheduling.quartz.SchedulerFactoryBean
import org.springframework.security.core.context.SecurityContextHolder
import org.springframework.web.client.RestClient
import org.springframework.web.client.RestTemplate
import org.springframework.web.reactive.function.client.WebClient
import org.springframework.web.servlet.HandlerExceptionResolver
Expand Down Expand Up @@ -638,6 +639,23 @@ class SentryAutoConfigurationTest {
}
}

@Test
fun `when tracing is enabled and RestClient is on the classpath, SentrySpanRestClientCustomizer bean is created`() {
contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj", "sentry.traces-sample-rate=1.0")
.run {
assertThat(it).hasSingleBean(SentrySpanRestClientCustomizer::class.java)
}
}

@Test
fun `when tracing is enabled and RestClient is not on the classpath, SentrySpanRestClientCustomizer bean is not created`() {
contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj", "sentry.traces-sample-rate=1.0")
.withClassLoader(FilteredClassLoader(RestClient::class.java))
.run {
assertThat(it).doesNotHaveBean(SentrySpanRestClientCustomizer::class.java)
}
}

@Test
fun `when tracing is enabled and WebClient is on the classpath, SentrySpanWebClientCustomizer bean is created`() {
contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj", "sentry.traces-sample-rate=1.0")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,288 @@
package io.sentry.spring.boot.jakarta

import io.sentry.*
import okhttp3.mockwebserver.MockResponse
import okhttp3.mockwebserver.MockWebServer
import okhttp3.mockwebserver.SocketPolicy
import org.assertj.core.api.Assertions.assertThat
import org.mockito.kotlin.*
import org.springframework.http.HttpHeaders
import org.springframework.http.HttpStatus
import org.springframework.http.client.HttpComponentsClientHttpRequestFactory
import org.springframework.web.client.RestClient
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
import kotlin.test.assertTrue

class SentrySpanRestClientCustomizerTest {
class Fixture {
val sentryOptions = SentryOptions()
val hub = mock<IHub>()
val restClientBuilder = RestClient.builder()
var mockServer = MockWebServer()
val transaction: SentryTracer
internal val customizer = SentrySpanRestClientCustomizer(hub)
val url = mockServer.url("/test/123").toString()
val scope = Scope(sentryOptions)

init {
whenever(hub.options).thenReturn(sentryOptions)
doAnswer { (it.arguments[0] as ScopeCallback).run(scope) }.whenever(hub).configureScope(any())
transaction = SentryTracer(TransactionContext("aTransaction", "op", TracesSamplingDecision(true)), hub)
}

fun getSut(isTransactionActive: Boolean, status: HttpStatus = HttpStatus.OK, socketPolicy: SocketPolicy = SocketPolicy.KEEP_OPEN, includeMockServerInTracingOrigins: Boolean = true): RestClient.Builder {
customizer.customize(restClientBuilder)

if (includeMockServerInTracingOrigins) {
sentryOptions.setTracePropagationTargets(listOf(mockServer.hostName))
} else {
sentryOptions.setTracePropagationTargets(listOf("other-api"))
}

sentryOptions.dsn = "https://key@sentry.io/proj"
sentryOptions.isTraceSampling = true

mockServer.enqueue(
MockResponse()
.setBody("OK")
.setSocketPolicy(socketPolicy)
.setResponseCode(status.value())
)

if (isTransactionActive) {
whenever(hub.span).thenReturn(transaction)
}

return restClientBuilder.apply{
val requestFactory = HttpComponentsClientHttpRequestFactory()
requestFactory.setConnectTimeout(2)
it.requestFactory(requestFactory)
}
}
}

private val fixture = Fixture()

@Test
fun `when transaction is active, creates span around RestClient HTTP call`() {
val result = fixture.getSut(isTransactionActive = true).build()
.get().uri(fixture.url).retrieve().toEntity(String::class.java)

assertThat(result.body).isEqualTo("OK")
assertThat(fixture.transaction.spans).hasSize(1)
val span = fixture.transaction.spans.first()
assertThat(span.operation).isEqualTo("http.client")
assertThat(span.description).isEqualTo("GET ${fixture.url}")
assertThat(span.status).isEqualTo(SpanStatus.OK)

val recordedRequest = fixture.mockServer.takeRequest()
assertThat(recordedRequest.headers["sentry-trace"]!!).startsWith(fixture.transaction.spanContext.traceId.toString())
.endsWith("-1")
.doesNotContain(fixture.transaction.spanContext.spanId.toString())
assertThat(recordedRequest.headers["baggage"]!!).contains(fixture.transaction.spanContext.traceId.toString())
}

@Test
fun `when there is an active span, existing baggage headers are merged with sentry baggage into single header`() {
val sut = fixture.getSut(isTransactionActive = true)
val headers = HttpHeaders()
headers.add("baggage", "thirdPartyBaggage=someValue")
headers.add("baggage", "secondThirdPartyBaggage=secondValue; property;propertyKey=propertyValue,anotherThirdPartyBaggage=anotherValue")

sut.build()
.get()
.uri(fixture.url)
.httpRequest { it.headers.addAll(headers) }
.retrieve()
.toEntity(String::class.java)

val recorderRequest = fixture.mockServer.takeRequest()
assertNotNull(recorderRequest.headers[SentryTraceHeader.SENTRY_TRACE_HEADER])
assertNotNull(recorderRequest.headers[BaggageHeader.BAGGAGE_HEADER])

val baggageHeaderValues = recorderRequest.headers.values(BaggageHeader.BAGGAGE_HEADER)
assertEquals(baggageHeaderValues.size, 1)
assertTrue(baggageHeaderValues[0].startsWith("thirdPartyBaggage=someValue,secondThirdPartyBaggage=secondValue; property;propertyKey=propertyValue,anotherThirdPartyBaggage=anotherValue"))
assertTrue(baggageHeaderValues[0].contains("sentry-public_key=key"))
assertTrue(baggageHeaderValues[0].contains("sentry-transaction=aTransaction"))
assertTrue(baggageHeaderValues[0].contains("sentry-trace_id"))
}

@Test
fun `when transaction is active and server is not listed in tracing origins, does not add sentry trace header to the request`() {
fixture.getSut(isTransactionActive = true, includeMockServerInTracingOrigins = false).build()
.get()
.uri(fixture.url)
.retrieve()
.toEntity(String::class.java)
val recordedRequest = fixture.mockServer.takeRequest()
assertThat(recordedRequest.headers[SentryTraceHeader.SENTRY_TRACE_HEADER]).isNull()
}

@Test
fun `when transaction is active and server is listed in tracing origins, adds sentry trace header to the request`() {
fixture.getSut(isTransactionActive = true, includeMockServerInTracingOrigins = true)
.build()
.get()
.uri(fixture.url)
.retrieve()
.toEntity(String::class.java)
val recordedRequest = fixture.mockServer.takeRequest()
assertThat(recordedRequest.headers[SentryTraceHeader.SENTRY_TRACE_HEADER]).isNotNull()
}

@Test
fun `when transaction is active and response code is not 2xx, creates span with error status around RestClient HTTP call`() {
try {
fixture.getSut(isTransactionActive = true, status = HttpStatus.INTERNAL_SERVER_ERROR)
.build()
.get()
.uri(fixture.url)
.retrieve()
.toEntity(String::class.java)
} catch (e: Throwable) {
}
assertThat(fixture.transaction.spans).hasSize(1)
val span = fixture.transaction.spans.first()
assertThat(span.operation).isEqualTo("http.client")
assertThat(span.description).isEqualTo("GET ${fixture.url}")
assertThat(span.status).isEqualTo(SpanStatus.INTERNAL_ERROR)
}

@Test
fun `when transaction is active and throws IO exception, creates span with error status around RestClient HTTP call`() {
nandorholozsnyak marked this conversation as resolved.
Show resolved Hide resolved
try {
fixture.getSut(isTransactionActive = true, socketPolicy = SocketPolicy.DISCONNECT_AT_START).build()
.get()
.uri(fixture.url)
.retrieve()
} catch (e: Throwable) {
}
fixture.mockServer.takeRequest()
assertThat(fixture.transaction.spans).hasSize(1)
val span = fixture.transaction.spans.first()
assertThat(span.operation).isEqualTo("http.client")
assertThat(span.description).isEqualTo("GET ${fixture.url}")
assertThat(span.status).isEqualTo(SpanStatus.INTERNAL_ERROR)
}

@Test
fun `when transaction is not active, does not create span around RestClient HTTP call`() {
val result = fixture.getSut(isTransactionActive = false).build()
.get()
.uri(fixture.url)
.retrieve()
.toEntity(String::class.java)

assertThat(result.body).isEqualTo("OK")
assertThat(fixture.transaction.spans).isEmpty()
}

@Test
fun `when transaction is not active, adds tracing headers from scope`() {
val sut = fixture.getSut(isTransactionActive = false)
val headers = HttpHeaders()
headers.add("baggage", "thirdPartyBaggage=someValue")
headers.add("baggage", "secondThirdPartyBaggage=secondValue; property;propertyKey=propertyValue,anotherThirdPartyBaggage=anotherValue")

sut.build()
.get()
.uri(fixture.url)
.httpRequest { it.headers.addAll(headers) }
.retrieve()
.toEntity(String::class.java)

val recorderRequest = fixture.mockServer.takeRequest()
assertNotNull(recorderRequest.headers[SentryTraceHeader.SENTRY_TRACE_HEADER])
assertNotNull(recorderRequest.headers[BaggageHeader.BAGGAGE_HEADER])

val baggageHeaderValues = recorderRequest.headers.values(BaggageHeader.BAGGAGE_HEADER)
assertEquals(baggageHeaderValues.size, 1)
assertTrue(baggageHeaderValues[0].startsWith("thirdPartyBaggage=someValue,secondThirdPartyBaggage=secondValue; property;propertyKey=propertyValue,anotherThirdPartyBaggage=anotherValue"))
assertTrue(baggageHeaderValues[0].contains("sentry-public_key=key"))
assertTrue(baggageHeaderValues[0].contains("sentry-trace_id"))
}

@Test
fun `when transaction is active adds breadcrumb when http calls succeeds`() {
fixture.getSut(isTransactionActive = true)
.build()
.post()
.uri(fixture.url)
.body("content")
.retrieve()
.toEntity(String::class.java)
verify(fixture.hub).addBreadcrumb(
check<Breadcrumb> {
assertEquals("http", it.type)
assertEquals(fixture.url, it.data["url"])
assertEquals("POST", it.data["method"])
assertEquals(7, it.data["request_body_size"])
},
anyOrNull()
)
}

@SuppressWarnings("SwallowedException")
@Test
fun `when transaction is active adds breadcrumb when http calls results in exception`() {
try {
fixture.getSut(isTransactionActive = true, status = HttpStatus.INTERNAL_SERVER_ERROR).build()
.get()
.uri(fixture.url)
.retrieve()
.toEntity(String::class.java)
} catch (e: Throwable) {
}
verify(fixture.hub).addBreadcrumb(
check<Breadcrumb> {
assertEquals("http", it.type)
assertEquals(fixture.url, it.data["url"])
assertEquals("GET", it.data["method"])
},
anyOrNull()
)
}

@Test
fun `when transaction is not active adds breadcrumb when http calls succeeds`() {
fixture.getSut(isTransactionActive = false) .build()
.post()
.uri(fixture.url)
.body("content")
.retrieve()
.toEntity(String::class.java)
verify(fixture.hub).addBreadcrumb(
check<Breadcrumb> {
assertEquals("http", it.type)
assertEquals(fixture.url, it.data["url"])
assertEquals("POST", it.data["method"])
assertEquals(7, it.data["request_body_size"])
},
anyOrNull()
)
}

@SuppressWarnings("SwallowedException")
@Test
fun `when transaction is not active adds breadcrumb when http calls results in exception`() {
try {
fixture.getSut(isTransactionActive = false, status = HttpStatus.INTERNAL_SERVER_ERROR).build()
.get()
.uri(fixture.url)
.retrieve()
.toEntity(String::class.java)
} catch (e: Throwable) {
}
verify(fixture.hub).addBreadcrumb(
check<Breadcrumb> {
assertEquals("http", it.type)
assertEquals(fixture.url, it.data["url"])
assertEquals("GET", it.data["method"])
},
anyOrNull()
)
}
}