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

Tracing without Performance #2788

Merged
merged 30 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
fdf5953
TwP
adinauer Jun 14, 2023
6eba5bc
fix tests
adinauer Jun 15, 2023
537b3fb
also add headers for okhttp if no span is active; add tests; fix exis…
adinauer Jun 19, 2023
686eb4e
Change trace and traceIfAllowed to return instead of taking a callback
adinauer Jun 19, 2023
62a3b39
Merge branch 'main' into feat/tracing-without-performance
adinauer Jun 19, 2023
4e850af
Start new trace in ActivityLifecycleIntegratin
adinauer Jun 19, 2023
15ad792
Also start a new trace in SentryNavigationListener
adinauer Jun 19, 2023
3cb799c
Add tests for continueTrace in spring filters
adinauer Jun 20, 2023
5feaac9
Add changelog
adinauer Jun 20, 2023
2bf3e3e
Merge branch 'main' into feat/tracing-without-performance
adinauer Jun 20, 2023
1b1f062
Add test for TracingUtils
adinauer Jun 20, 2023
66e4bdb
More tests
adinauer Jun 20, 2023
b6bfe7e
Switch from AtomicReference to holder class
adinauer Jun 22, 2023
104ac37
Format code
getsentry-bot Jun 22, 2023
2266d58
Changes according to reviews
adinauer Jun 22, 2023
13a6a38
Merge branch 'main' into feat/tracing-without-performance
adinauer Jun 22, 2023
0bb6886
Start new trace in SentryGestureListener
adinauer Jun 22, 2023
49b1699
More tests
adinauer Jun 23, 2023
e7e50bd
Synchronize on scope for trace creation and usage from scope
adinauer Jun 26, 2023
4e5f6aa
Add withPropagationContext to Scope and synchronize using a private lock
adinauer Jun 26, 2023
1ec1b5d
Default to false for sampled decision of incoming trace without expli…
adinauer Jun 26, 2023
ad3d2a4
Extra variable
adinauer Jun 26, 2023
7db620b
Apply suggestions from code review
adinauer Jun 27, 2023
38956f1
Mark PropagationContext internal
adinauer Jun 27, 2023
ceb2132
Merge branch 'main' into feat/tracing-without-performance
adinauer Jun 29, 2023
ef08924
move changelog
adinauer Jun 29, 2023
747ae3b
Update sentry-android-core/src/main/java/io/sentry/android/core/Activ…
adinauer Jun 29, 2023
2eb2140
Format code
getsentry-bot Jun 29, 2023
23dcd1a
refrain from starting a new trace in some cases
adinauer Jun 29, 2023
a360064
Apply suggestions from code review
adinauer Jun 30, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ private void stopPreviousTransactions() {
}
}

// TODO should we start a new trace here if performance is disabled?
adinauer marked this conversation as resolved.
Show resolved Hide resolved
private void startTracing(final @NotNull Activity activity) {
WeakReference<Activity> weakActivity = new WeakReference<>(activity);
if (performanceEnabled && !isRunningTransaction(activity) && hub != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ private void addBreadcrumb(
hint);
}

// TODO should we start a new trace here if performance is disabled?
adinauer marked this conversation as resolved.
Show resolved Hide resolved
private void startTracing(final @NotNull UiElement target, final @NotNull String eventType) {
if (!(options.isTracingEnabled() && options.isEnableUserInteractionTracing())) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class SentryNavigationListener @JvmOverloads constructor(
hub.addBreadcrumb(breadcrumb, hint)
}

// TODO should we start a new trace here if performance is disabled?
adinauer marked this conversation as resolved.
Show resolved Hide resolved
private fun startTracing(
controller: NavController,
destination: NavDestination,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import io.sentry.exception.SentryHttpClientException
import io.sentry.protocol.Mechanism
import io.sentry.util.HttpUtils
import io.sentry.util.PropagationTargetsUtils
import io.sentry.util.TracingUtils
import io.sentry.util.UrlUtils
import okhttp3.Headers
import okhttp3.Interceptor
Expand Down Expand Up @@ -85,18 +86,20 @@ class SentryOkHttpInterceptor(
var code: Int? = null
try {
val requestBuilder = request.newBuilder()
if (span != null && !span.isNoOp &&
PropagationTargetsUtils.contain(hub.options.tracePropagationTargets, request.url.toString())
) {
span.toSentryTrace().let {
requestBuilder.addHeader(it.name, it.value)
}

span.toBaggageHeader(request.headers(BaggageHeader.BAGGAGE_HEADER))?.let {
TracingUtils.traceIfAllowed(
hub,
request.url.toString(),
request.headers(BaggageHeader.BAGGAGE_HEADER),
span
) { tracingHeaders ->
requestBuilder.addHeader(tracingHeaders.sentryTraceHeader.name, tracingHeaders.sentryTraceHeader.value)
tracingHeaders.baggageHeader?.let {
requestBuilder.removeHeader(BaggageHeader.BAGGAGE_HEADER)
requestBuilder.addHeader(it.name, it.value)
}
}

request = requestBuilder.build()
response = chain.proceed(request)
code = response.code
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import io.sentry.SentryIntegrationPackageStorage
import io.sentry.SentryLevel
import io.sentry.SpanStatus
import io.sentry.TypeCheckHint
import io.sentry.util.PropagationTargetsUtils
import io.sentry.util.TracingUtils
import io.sentry.util.UrlUtils
import io.sentry.vendor.Base64

Expand All @@ -42,14 +42,11 @@ class SentryApollo3HttpInterceptor @JvmOverloads constructor(private val hub: IH

var cleanedHeaders = removeSentryInternalHeaders(request.headers).toMutableList()

if (!span.isNoOp && PropagationTargetsUtils.contain(hub.options.tracePropagationTargets, request.url)) {
val sentryTraceHeader = span.toSentryTrace()
val baggageHeader = span.toBaggageHeader(request.headers.filter { it.name == BaggageHeader.BAGGAGE_HEADER }.map { it.value })
cleanedHeaders.add(HttpHeader(sentryTraceHeader.name, sentryTraceHeader.value))

baggageHeader?.let { newHeader ->
TracingUtils.traceIfAllowed(hub, request.url, request.headers.filter { it.name == BaggageHeader.BAGGAGE_HEADER }.map { it.value }, span) {
cleanedHeaders.add(HttpHeader(it.sentryTraceHeader.name, it.sentryTraceHeader.value))
it.baggageHeader?.let { baggageHeader ->
cleanedHeaders = cleanedHeaders.filterNot { it.name == BaggageHeader.BAGGAGE_HEADER }.toMutableList().apply {
add(HttpHeader(newHeader.name, newHeader.value))
add(HttpHeader(baggageHeader.name, baggageHeader.value))
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import com.apollographql.apollo.interceptor.ApolloInterceptor.FetchSourceType
import com.apollographql.apollo.interceptor.ApolloInterceptor.InterceptorRequest
import com.apollographql.apollo.interceptor.ApolloInterceptor.InterceptorResponse
import com.apollographql.apollo.interceptor.ApolloInterceptorChain
import com.apollographql.apollo.request.RequestHeaders
import io.sentry.BaggageHeader
import io.sentry.Breadcrumb
import io.sentry.Hint
Expand All @@ -23,6 +24,7 @@ import io.sentry.SentryLevel
import io.sentry.SpanStatus
import io.sentry.TypeCheckHint.APOLLO_REQUEST
import io.sentry.TypeCheckHint.APOLLO_RESPONSE
import io.sentry.util.TracingUtils
import java.util.concurrent.Executor

class SentryApolloInterceptor(
Expand All @@ -41,25 +43,14 @@ class SentryApolloInterceptor(
override fun interceptAsync(request: InterceptorRequest, chain: ApolloInterceptorChain, dispatcher: Executor, callBack: CallBack) {
val activeSpan = hub.span
if (activeSpan == null) {
chain.proceedAsync(request, dispatcher, callBack)
val headers = addTracingHeaders(request, null)
val modifiedRequest = request.toBuilder().requestHeaders(headers).build()
chain.proceedAsync(modifiedRequest, dispatcher, callBack)
} else {
val span = startChild(request, activeSpan)

val requestWithHeader = if (span.isNoOp) {
request
} else {
val sentryTraceHeader = span.toSentryTrace()

// we have no access to URI, no way to verify tracing origins
val requestHeaderBuilder = request.requestHeaders.toBuilder()
requestHeaderBuilder.addHeader(sentryTraceHeader.name, sentryTraceHeader.value)
span.toBaggageHeader(listOf(request.requestHeaders.headerValue(BaggageHeader.BAGGAGE_HEADER)))
?.let {
requestHeaderBuilder.addHeader(it.name, it.value)
}
val headers = requestHeaderBuilder.build()
request.toBuilder().requestHeaders(headers).build()
}
val headers = addTracingHeaders(request, span)
val requestWithHeader = request.toBuilder().requestHeaders(headers).build()

span.setData("operationId", requestWithHeader.operation.operationId())
span.setData("variables", requestWithHeader.operation.variables().valueMap().toString())
Expand Down Expand Up @@ -100,6 +91,29 @@ class SentryApolloInterceptor(

override fun dispose() {}

private fun addTracingHeaders(request: InterceptorRequest, span: ISpan?): RequestHeaders {
val requestHeaderBuilder = request.requestHeaders.toBuilder()

if (hub.options.isTraceSampling) {
// we have no access to URI, no way to verify tracing origins
TracingUtils.trace(
hub,
listOf(request.requestHeaders.headerValue(BaggageHeader.BAGGAGE_HEADER)),
span
) { tracingHeaders ->
requestHeaderBuilder.addHeader(
tracingHeaders.sentryTraceHeader.name,
tracingHeaders.sentryTraceHeader.value
)
tracingHeaders.baggageHeader?.let {
requestHeaderBuilder.addHeader(it.name, it.value)
}
}
}

return requestHeaderBuilder.build()
}

private fun startChild(request: InterceptorRequest, activeSpan: ISpan): ISpan {
val operation = request.operation.name().name()
val operationType = when (request.operation) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@
import io.sentry.Hint;
import io.sentry.IHub;
import io.sentry.ISpan;
import io.sentry.SentryTraceHeader;
import io.sentry.SpanStatus;
import io.sentry.util.Objects;
import io.sentry.util.PropagationTargetsUtils;
import io.sentry.util.TracingUtils;
import io.sentry.util.UrlUtils;
import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -46,35 +45,21 @@ public Response execute(final @NotNull Request request, final @NotNull Request.O
Response response = null;
try {
final ISpan activeSpan = hub.getSpan();

if (activeSpan == null) {
return delegate.execute(request, options);
final @NotNull Request modifiedRequest = maybeAddTracingHeaders(request, null);
return delegate.execute(modifiedRequest, options);
}

ISpan span = activeSpan.startChild("http.client");
final @NotNull UrlUtils.UrlDetails urlDetails = UrlUtils.parse(request.url());
span.setDescription(request.httpMethod().name() + " " + urlDetails.getUrlOrFallback());
urlDetails.applyToSpan(span);

final RequestWrapper requestWrapper = new RequestWrapper(request);

if (!span.isNoOp()
&& PropagationTargetsUtils.contain(
hub.getOptions().getTracePropagationTargets(), request.url())) {
final SentryTraceHeader sentryTraceHeader = span.toSentryTrace();
final @Nullable Collection<String> requestBaggageHeader =
request.headers().get(BaggageHeader.BAGGAGE_HEADER);
final @Nullable BaggageHeader baggageHeader =
span.toBaggageHeader(
requestBaggageHeader != null ? new ArrayList<>(requestBaggageHeader) : null);
requestWrapper.header(sentryTraceHeader.getName(), sentryTraceHeader.getValue());
if (baggageHeader != null) {
requestWrapper.removeHeader(BaggageHeader.BAGGAGE_HEADER);
requestWrapper.header(baggageHeader.getName(), baggageHeader.getValue());
}
}
final @NotNull Request modifiedRequest = maybeAddTracingHeaders(request, span);

try {
response = delegate.execute(requestWrapper.build(), options);
response = delegate.execute(modifiedRequest, options);
// handles both success and error responses
span.setStatus(SpanStatus.fromHttpStatusCode(response.status()));
return response;
Expand Down Expand Up @@ -102,6 +87,30 @@ public Response execute(final @NotNull Request request, final @NotNull Request.O
}
}

private @NotNull Request maybeAddTracingHeaders(
final @NotNull Request request, final @Nullable ISpan span) {
final RequestWrapper requestWrapper = new RequestWrapper(request);

TracingUtils.traceIfAllowed(
hub,
request.url(),
new ArrayList<>(request.headers().get(BaggageHeader.BAGGAGE_HEADER)),
span,
tracingHeaders -> {
requestWrapper.header(
tracingHeaders.getSentryTraceHeader().getName(),
tracingHeaders.getSentryTraceHeader().getValue());

final @Nullable BaggageHeader baggageHeader = tracingHeaders.getBaggageHeader();
if (baggageHeader != null) {
requestWrapper.removeHeader(BaggageHeader.BAGGAGE_HEADER);
requestWrapper.header(baggageHeader.getName(), baggageHeader.getValue());
}
});

return requestWrapper.build();
}

private void addBreadcrumb(final @NotNull Request request, final @Nullable Response response) {
final Breadcrumb breadcrumb =
Breadcrumb.http(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import io.sentry.ISpan;
import io.sentry.ITransaction;
import io.sentry.Instrumenter;
import io.sentry.PropagationContext;
import io.sentry.SentryDate;
import io.sentry.SentryLevel;
import io.sentry.SentryLongDate;
Expand Down Expand Up @@ -113,13 +114,12 @@ public void onStart(final @NotNull Context parentContext, final @NotNull ReadWri
null,
null,
null)
: TransactionContext.fromSentryTrace(
: TransactionContext.fromPropagationContext(
transactionName,
transactionNameSource,
op,
traceData.getSentryTraceHeader(),
traceData.getBaggage(),
spanId);
PropagationContext.fromHeaders(
traceData.getSentryTraceHeader(), traceData.getBaggage(), spanId));
;
transactionContext.setInstrumenter(Instrumenter.OTEL);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,6 @@ static class SentrySecurityConfiguration {
}

@Bean
@Conditional(SentryTracingCondition.class)
@ConditionalOnMissingBean(name = "sentryTracingFilter")
public FilterRegistrationBean<SentryTracingFilter> sentryTracingFilter(
final @NotNull IHub hub, final @NotNull TransactionNameProvider transactionNameProvider) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -458,18 +458,10 @@ class SentryAutoConfigurationTest {
}

@Test
fun `when tracing is set, does not create tracing filter`() {
fun `creates tracing filter`() {
contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj")
.run {
assertThat(it).doesNotHaveBean("sentryTracingFilter")
}
}

@Test
fun `when tracing is disabled, does not create tracing filter`() {
contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj")
.run {
assertThat(it).doesNotHaveBean("sentryTracingFilter")
assertThat(it).hasBean("sentryTracingFilter")
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,6 @@ static class SentrySecurityConfiguration {
}

@Bean
@Conditional(SentryTracingCondition.class)
@ConditionalOnMissingBean(name = "sentryTracingFilter")
public FilterRegistrationBean<SentryTracingFilter> sentryTracingFilter(
final @NotNull IHub hub, final @NotNull TransactionNameProvider transactionNameProvider) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -458,18 +458,10 @@ class SentryAutoConfigurationTest {
}

@Test
fun `when tracing is set, does not create tracing filter`() {
fun `creates tracing filter`() {
contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj")
.run {
assertThat(it).doesNotHaveBean("sentryTracingFilter")
}
}

@Test
fun `when tracing is disabled, does not create tracing filter`() {
contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj")
.run {
assertThat(it).doesNotHaveBean("sentryTracingFilter")
assertThat(it).hasBean("sentryTracingFilter")
}
}

Expand Down
2 changes: 1 addition & 1 deletion sentry-spring-jakarta/api/sentry-spring-jakarta.api
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ public abstract class io/sentry/spring/jakarta/webflux/AbstractSentryWebFilter :
protected fun doOnError (Lio/sentry/ITransaction;Ljava/lang/Throwable;)V
protected fun maybeStartTransaction (Lio/sentry/IHub;Lorg/springframework/http/server/reactive/ServerHttpRequest;)Lio/sentry/ITransaction;
protected fun shouldTraceRequest (Lio/sentry/IHub;Lorg/springframework/http/server/reactive/ServerHttpRequest;)Z
protected fun startTransaction (Lio/sentry/IHub;Lorg/springframework/http/server/reactive/ServerHttpRequest;)Lio/sentry/ITransaction;
protected fun startTransaction (Lio/sentry/IHub;Lorg/springframework/http/server/reactive/ServerHttpRequest;Lio/sentry/PropagationContext;)Lio/sentry/ITransaction;
}

public final class io/sentry/spring/jakarta/webflux/ReactorUtils {
Expand Down
Loading