fix(twp): Trace propagation in HTTP tracing clients

CHANGELOG.md

@@ -10,6 +10,10 @@
 Sentry.addFeatureFlag('my-feature', true);
 ```
 
+### Fixes
+
+- Trace propagation in HTTP tracing clients not correctly set up if performance is disabled ([#2850](https://github.com/getsentry/sentry-dart/pull/2850))
+
 ### Behavioral changes
 
 - Mutable Data Classes ([#2818](https://github.com/getsentry/sentry-dart/pull/2818))

dart/lib/sentry.dart

@@ -24,8 +24,8 @@ export 'src/noop_isolate_error_integration.dart'
 export 'src/observers.dart';
 export 'src/performance_collector.dart';
 export 'src/protocol.dart';
-export 'src/protocol/sentry_feature_flags.dart';
 export 'src/protocol/sentry_feature_flag.dart';
+export 'src/protocol/sentry_feature_flags.dart';
 export 'src/protocol/sentry_feedback.dart';
 export 'src/protocol/sentry_proxy.dart';
 export 'src/run_zoned_guarded_integration.dart';
@@ -50,6 +50,8 @@ export 'src/type_check_hint.dart';
 // ignore: invalid_export_of_internal_element
 export 'src/utils.dart';
 // ignore: invalid_export_of_internal_element
+export 'src/utils/add_tracing_headers_to_http_request.dart';
+// ignore: invalid_export_of_internal_element
 export 'src/utils/http_header_utils.dart';
 // ignore: invalid_export_of_internal_element
 export 'src/utils/http_sanitizer.dart';

dart/lib/src/http_client/sentry_http_client.dart

@@ -1,9 +1,10 @@
 import 'package:http/http.dart';
-import 'tracing_client.dart';
+
 import '../hub.dart';
 import '../hub_adapter.dart';
 import 'breadcrumb_client.dart';
 import 'failed_request_client.dart';
+import 'tracing_client.dart';
 
 /// A [http](https://pub.dev/packages/http)-package compatible HTTP client.
 ///
@@ -103,10 +104,7 @@ class SentryHttpClient extends BaseClient {
       captureFailedRequests: captureFailedRequests,
     );
 
-    if (_hub.options.isTracingEnabled()) {
-      innerClient = TracingClient(client: innerClient, hub: _hub);
-      _hub.options.sdk.addIntegration('HTTPNetworkTracing');
-    }
+    innerClient = TracingClient(client: innerClient, hub: _hub);
 
     // The ordering here matters.
     // We don't want to include the breadcrumbs for the current request

dart/lib/src/http_client/tracing_client.dart

@@ -1,27 +1,36 @@
 import 'package:http/http.dart';
+
 import '../hub.dart';
 import '../hub_adapter.dart';
 import '../protocol.dart';
 import '../sentry_trace_origins.dart';
 import '../tracing.dart';
-import '../utils/tracing_utils.dart';
+import '../utils/add_tracing_headers_to_http_request.dart';
 import '../utils/http_sanitizer.dart';
+import '../utils/tracing_utils.dart';
 
 /// A [http](https://pub.dev/packages/http)-package compatible HTTP client
-/// which adds support to Sentry Performance feature.
+/// which adds support to Sentry Performance feature. If tracing is disabled
+/// generated spans will be no-op. This client also handles adding the
+/// of Sentry trace headers to the request header.
 /// https://develop.sentry.dev/sdk/performance
 class TracingClient extends BaseClient {
+  static const String integrationName = 'HTTPNetworkTracing';
+
   TracingClient({Client? client, Hub? hub})
       : _hub = hub ?? HubAdapter(),
-        _client = client ?? Client();
+        _client = client ?? Client() {
+    if (_hub.options.isTracingEnabled()) {
+      _hub.options.sdk.addIntegration(integrationName);
+    }
+  }
 
   final Client _client;
   final Hub _hub;
 
   @override
   Future<StreamedResponse> send(BaseRequest request) async {
     // see https://develop.sentry.dev/sdk/performance/#header-sentry-trace
-
     final urlDetails = HttpSanitizer.sanitizeUrl(request.url.toString());
 
     var description = request.method;
@@ -34,43 +43,24 @@
       'http.client',
       description: description,
     );
-    span?.origin = SentryTraceOrigins.autoHttpHttp;
 
-    // if the span is NoOp, we don't want to attach headers
     if (span is NoOpSentrySpan) {
       span = null;
     }
 
+    // Regardless whether tracing is enabled or not, we always want to attach
+    // Sentry trace headers (tracing without performance).
+    if (containsTargetOrMatchesRegExp(
+        _hub.options.tracePropagationTargets, request.url.toString())) {
+      addTracingHeadersToHttpHeader(request.headers, span: span, hub: _hub);
+    }
+
+    span?.origin = SentryTraceOrigins.autoHttpHttp;
     span?.setData('http.request.method', request.method);
     urlDetails?.applyToSpan(span);
 
     StreamedResponse? response;
     try {
-      if (containsTargetOrMatchesRegExp(
-          _hub.options.tracePropagationTargets, request.url.toString())) {
-        if (span != null) {
-          addSentryTraceHeaderFromSpan(span, request.headers);
-          addBaggageHeaderFromSpan(
-            span,
-            request.headers,
-            logger: _hub.options.logger,
-          );
-        } else {
-          final scope = _hub.scope;
-          final propagationContext = scope.propagationContext;
-
-          final traceHeader = propagationContext.toSentryTrace();
-          addSentryTraceHeader(traceHeader, request.headers);
-
-          final baggage = propagationContext.baggage;
-          if (baggage != null) {
-            final baggageHeader = SentryBaggageHeader.fromBaggage(baggage);
-            addBaggageHeader(baggageHeader, request.headers,
-                logger: _hub.options.logger);
-          }
-        }
-      }
-
       response = await _client.send(request);
       span?.setData('http.response.status_code', response.statusCode);
       span?.setData('http.response_content_length', response.contentLength);

dart/lib/src/propagation_context.dart

@@ -1,4 +1,5 @@
 import 'package:meta/meta.dart';
+
 import 'protocol.dart';
 import 'sentry_baggage.dart';
 
@@ -8,5 +9,7 @@ class PropagationContext {
   late SpanId spanId = SpanId.newId();
   SentryBaggage? baggage;
 
+  SentryBaggageHeader? toBaggageHeader() =>
+      baggage != null ? SentryBaggageHeader.fromBaggage(baggage!) : null;
   SentryTraceHeader toSentryTrace() => SentryTraceHeader(traceId, spanId);
 }

dart/lib/src/utils/add_tracing_headers_to_http_request.dart

@@ -0,0 +1,29 @@
+import 'package:meta/meta.dart';
+
+import '../../sentry.dart';
+
+@internal
+void addTracingHeadersToHttpHeader(Map<String, dynamic> headers,
+    {ISentrySpan? span, Hub? hub}) {
+  hub ??= Sentry.currentHub;
+
+  if (span != null) {
+    addSentryTraceHeaderFromSpan(span, headers);
+    addBaggageHeaderFromSpan(
+      span,
+      headers,
+      logger: hub.options.logger,
+    );
+  } else {
+    final scope = hub.scope;
+    final propagationContext = scope.propagationContext;
+
+    final traceHeader = propagationContext.toSentryTrace();
+    addSentryTraceHeader(traceHeader, headers);
+
+    final baggageHeader = propagationContext.toBaggageHeader();
+    if (baggageHeader != null) {
+      addBaggageHeader(baggageHeader, headers, logger: hub.options.logger);
+    }
+  }
+}

dart/test/http_client/sentry_http_client_test.dart

@@ -3,9 +3,12 @@ import 'package:http/testing.dart';
 import 'package:mockito/mockito.dart';
 import 'package:sentry/sentry.dart';
 import 'package:sentry/src/http_client/failed_request_client.dart';
+import 'package:sentry/src/sentry_tracer.dart';
 import 'package:test/test.dart';
 
 import '../mocks/mock_hub.dart';
+import '../mocks/mock_transport.dart';
+import '../test_utils.dart';
 
 final requestUri = Uri.parse('https://example.com');
 
@@ -27,25 +30,25 @@ void main() {
       final response = await sut.get(requestUri);
       expect(response.statusCode, 200);
 
-      expect(fixture.hub.captureEventCalls.length, 0);
-      expect(fixture.hub.addBreadcrumbCalls.length, 1);
+      expect(fixture.mockHub.captureEventCalls.length, 0);
+      expect(fixture.mockHub.addBreadcrumbCalls.length, 1);
     });
 
     test('no captured event with default config', () async {
-      fixture.hub.options.captureFailedRequests = false;
+      fixture.mockHub.options.captureFailedRequests = false;
 
       final sut = fixture.getSut(
         client: createThrowingClient(),
       );
 
       await expectLater(() async => await sut.get(requestUri), throwsException);
 
-      expect(fixture.hub.captureEventCalls.length, 0);
-      expect(fixture.hub.addBreadcrumbCalls.length, 1);
+      expect(fixture.mockHub.captureEventCalls.length, 0);
+      expect(fixture.mockHub.addBreadcrumbCalls.length, 1);
     });
 
     test('captured event with override', () async {
-      fixture.hub.options.captureFailedRequests = false;
+      fixture.mockHub.options.captureFailedRequests = false;
 
       final sut = fixture.getSut(
         client: createThrowingClient(),
@@ -54,39 +57,39 @@ void main() {
 
       await expectLater(() async => await sut.get(requestUri), throwsException);
 
-      expect(fixture.hub.captureEventCalls.length, 1);
+      expect(fixture.mockHub.captureEventCalls.length, 1);
     });
 
     test('one captured event with when enabling $FailedRequestClient',
         () async {
-      fixture.hub.options.captureFailedRequests = true;
-      fixture.hub.options.recordHttpBreadcrumbs = true;
+      fixture.mockHub.options.captureFailedRequests = true;
+      fixture.mockHub.options.recordHttpBreadcrumbs = true;
       final sut = fixture.getSut(
         client: createThrowingClient(),
       );
 
       await expectLater(() async => await sut.get(requestUri), throwsException);
 
-      expect(fixture.hub.captureEventCalls.length, 1);
+      expect(fixture.mockHub.captureEventCalls.length, 1);
       // The event should not have breadcrumbs from the BreadcrumbClient
-      expect(fixture.hub.captureEventCalls.first.event.breadcrumbs, null);
+      expect(fixture.mockHub.captureEventCalls.first.event.breadcrumbs, null);
       // The breadcrumb for the request should still be added for every
       // following event.
-      expect(fixture.hub.addBreadcrumbCalls.length, 1);
+      expect(fixture.mockHub.addBreadcrumbCalls.length, 1);
     });
 
     test(
         'no captured event with when enabling $FailedRequestClient with override',
         () async {
-      fixture.hub.options.captureFailedRequests = true;
+      fixture.mockHub.options.captureFailedRequests = true;
       final sut = fixture.getSut(
         client: createThrowingClient(),
         captureFailedRequests: false,
       );
 
       await expectLater(() async => await sut.get(requestUri), throwsException);
 
-      expect(fixture.hub.captureEventCalls.length, 0);
+      expect(fixture.mockHub.captureEventCalls.length, 0);
     });
 
     test('close does get called for user defined client', () async {
@@ -103,28 +106,45 @@ void main() {
     });
 
     test('no captured span if tracing disabled', () async {
-      fixture.hub.options.recordHttpBreadcrumbs = false;
+      fixture.realHub.options.recordHttpBreadcrumbs = false;
+      final tr = fixture.realHub.startTransaction(
+        'name',
+        'op',
+        bindToScope: true,
+      );
+
       final sut = fixture.getSut(
+        hub: fixture.realHub,
         client: fixture.getClient(statusCode: 200, reason: 'OK'),
       );
-
       final response = await sut.get(requestUri);
-      expect(response.statusCode, 200);
 
-      expect(fixture.hub.getSpanCalls, 0);
+      await tr.finish();
+
+      expect(response.statusCode, 200);
+      expect(tr, isA<NoOpSentrySpan>());
     });
 
     test('captured span if tracing enabled', () async {
-      fixture.hub.options.tracesSampleRate = 1.0;
-      fixture.hub.options.recordHttpBreadcrumbs = false;
+      fixture.realHub.options.tracesSampleRate = 1.0;
+      fixture.realHub.options.recordHttpBreadcrumbs = false;
+      final tr = fixture.realHub.startTransaction(
+        'name',
+        'op',
+        bindToScope: true,
+      ) as SentryTracer;
+
       final sut = fixture.getSut(
         client: fixture.getClient(statusCode: 200, reason: 'OK'),
+        hub: fixture.realHub,
       );
-
       final response = await sut.get(requestUri);
-      expect(response.statusCode, 200);
 
-      expect(fixture.hub.getSpanCalls, 1);
+      await tr.finish();
+
+      expect(response.statusCode, 200);
+      expect(tr.children.length, 1);
+      expect(tr.children.first.context.operation, 'http.client');
     });
   });
 }
@@ -141,12 +161,27 @@ MockClient createThrowingClient() {
 class CloseableMockClient extends Mock implements BaseClient {}
 
 class Fixture {
+  late MockHub mockHub;
+  late Hub realHub;
+  late MockTransport transport;
+  final options = defaultTestOptions();
+
+  Fixture() {
+    // For some tests the real hub is needed, for other the mock is enough
+    transport = MockTransport();
+    options.transport = transport;
+    realHub = Hub(options);
+    mockHub = MockHub();
+  }
+
   SentryHttpClient getSut({
     MockClient? client,
     List<SentryStatusCode> badStatusCodes = const [],
     bool? captureFailedRequests,
+    Hub? hub,
   }) {
     final mc = client ?? getClient();
+    hub ??= mockHub;
     return SentryHttpClient(
       client: mc,
       hub: hub,
@@ -155,8 +190,6 @@ class Fixture {
     );
   }
 
-  final MockHub hub = MockHub();
-
   MockClient getClient({int statusCode = 200, String? reason}) {
     return MockClient((request) async {
       expect(request.url, requestUri);