-
-
Notifications
You must be signed in to change notification settings - Fork 284
fix(tracing): Generate new trace on navigation #2861
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
Changes from 20 commits
ebb3088
4fd3a4b
bdb2cf6
5b85a73
cbba03b
3923a31
d17cc58
d4fa389
e2393b2
fad1ee4
9d34dd1
5a0c06d
d34ee4a
f417fc7
482772e
d301aeb
48ae70f
3f5ea16
956230c
160289a
efcca5f
ea6d3d1
c782130
2748fd4
3e89a70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ import 'package:meta/meta.dart'; | |
| import '../sentry.dart'; | ||
| import 'client_reports/discard_reason.dart'; | ||
| import 'profiling.dart'; | ||
| import 'propagation_context.dart'; | ||
| import 'sentry_tracer.dart'; | ||
| import 'sentry_traces_sampler.dart'; | ||
| import 'transport/data_category.dart'; | ||
|
|
@@ -454,9 +453,6 @@ class Hub { | |
| SentryLevel.warning, | ||
| "Instance is disabled and this 'startTransaction' call is a no-op.", | ||
| ); | ||
| } else if (!_options.isTracingEnabled()) { | ||
| final item = _peek(); | ||
| item.scope.propagationContext = PropagationContext(); | ||
| } else { | ||
| final item = _peek(); | ||
|
|
||
|
|
@@ -470,6 +466,7 @@ class Hub { | |
| } | ||
|
|
||
| transactionContext.origin ??= SentryTraceOrigins.manual; | ||
| transactionContext.traceId = scope.propagationContext.traceId; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. navigator observer will take care of updating the propagation context so new transactions just need to use it and child spans will inherit the trace id from parent
buenaflor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| SentryProfiler? profiler; | ||
| if (_profilerFactory != null && | ||
|
|
@@ -497,6 +494,11 @@ class Hub { | |
| return NoOpSentrySpan(); | ||
| } | ||
|
|
||
| @internal | ||
| void generateNewTraceId() { | ||
| scope.propagationContext.traceId = SentryId.newId(); | ||
buenaflor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /// Gets the current active transaction or span. | ||
| ISentrySpan? getSpan() { | ||
| ISentrySpan? span; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,17 @@ class Scope { | |
| /// Returns active transaction or null if there is no active transaction. | ||
| ISentrySpan? span; | ||
|
|
||
| /// The propagation context for connecting errors and spans to traces. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love the additional descriptions/documetnations you have added. ❤️ |
||
| /// There should always be a propagation context available at all times. | ||
| /// | ||
| /// Default behaviour of trace generation in Flutter: | ||
| /// | ||
| /// If `SentryNavigatorObserver` is available: | ||
| /// - new trace on navigation for all platforms | ||
| /// | ||
| /// if `SentryNavigatorObserver` is not available: | ||
| /// - Mobile: traces will be cycled with the background/foreground hooks, similar to how sessions are defined in mobile | ||
| /// - Web: traces will stick until the next refresh (this might change in the future) | ||
| @internal | ||
| PropagationContext propagationContext = PropagationContext(); | ||
|
|
||
|
|
@@ -246,6 +257,7 @@ class Scope { | |
| _extra.clear(); | ||
| _eventProcessors.clear(); | ||
| _replayId = null; | ||
| propagationContext = PropagationContext(); | ||
|
|
||
| _clearBreadcrumbsSync(); | ||
| _setUserSync(null); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| // ignore_for_file: invalid_use_of_internal_member | ||
|
|
||
| import 'package:flutter/cupertino.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:mockito/mockito.dart'; | ||
| import 'package:sentry_flutter/sentry_flutter.dart'; | ||
|
|
||
| import '../mocks.dart'; | ||
| import '../mocks.mocks.dart'; | ||
|
|
||
| void main() { | ||
| late Fixture fixture; | ||
|
|
||
| setUp(() { | ||
| fixture = Fixture(); | ||
| }); | ||
|
|
||
| test('didPush generates a new trace', () { | ||
| final fromRoute = _route(RouteSettings(name: 'From Route')); | ||
| final toRoute = _route(RouteSettings(name: 'To Route')); | ||
| final oldTraceId = fixture.hub.scope.propagationContext.traceId; | ||
|
|
||
| final sut = fixture.getSut(); | ||
| sut.didPush(toRoute, fromRoute); | ||
|
|
||
| final newTraceId = fixture.hub.scope.propagationContext.traceId; | ||
| expect(oldTraceId, isNot(newTraceId)); | ||
| }); | ||
|
|
||
| test('didPop generates a new trace', () { | ||
| final fromRoute = _route(RouteSettings(name: 'From Route')); | ||
| final toRoute = _route(RouteSettings(name: 'To Route')); | ||
| final oldTraceId = fixture.hub.scope.propagationContext.traceId; | ||
|
|
||
| final sut = fixture.getSut(); | ||
| sut.didPop(toRoute, fromRoute); | ||
|
|
||
| final newTraceId = fixture.hub.scope.propagationContext.traceId; | ||
| expect(oldTraceId, isNot(newTraceId)); | ||
| }); | ||
|
|
||
| test('didReplace generates a new trace', () { | ||
| final fromRoute = _route(RouteSettings(name: 'From Route')); | ||
| final toRoute = _route(RouteSettings(name: 'To Route')); | ||
| final oldTraceId = fixture.hub.scope.propagationContext.traceId; | ||
|
|
||
| final sut = fixture.getSut(); | ||
| sut.didReplace(newRoute: toRoute, oldRoute: fromRoute); | ||
|
|
||
| final newTraceId = fixture.hub.scope.propagationContext.traceId; | ||
| expect(oldTraceId, isNot(newTraceId)); | ||
| }); | ||
|
|
||
| group('execution order', () { | ||
| /// Prepares mocks, we don't care about what they exactly do. | ||
| /// We only test the order of execution in this group. | ||
| void _prepareMocks() { | ||
| when(fixture.mockHub.generateNewTraceId()).thenAnswer((_) => {}); | ||
| when(fixture.mockHub.configureScope(any)) | ||
| .thenAnswer((_) => Future.value()); | ||
| when(fixture.mockHub.startTransactionWithContext( | ||
| any, | ||
| bindToScope: anyNamed('bindToScope'), | ||
| waitForChildren: anyNamed('waitForChildren'), | ||
| autoFinishAfter: anyNamed('autoFinishAfter'), | ||
| trimEnd: anyNamed('trimEnd'), | ||
| onFinish: anyNamed('onFinish'), | ||
| customSamplingContext: anyNamed('customSamplingContext'), | ||
| startTimestamp: anyNamed('startTimestamp'), | ||
| )).thenReturn(NoOpSentrySpan()); | ||
| } | ||
|
|
||
| test('didPush generates a new trace before creating transaction spans', () { | ||
| final fromRoute = _route(RouteSettings(name: 'From Route')); | ||
| final toRoute = _route(RouteSettings(name: 'To Route')); | ||
|
|
||
| _prepareMocks(); | ||
|
|
||
| final sut = fixture.getSut(hub: fixture.mockHub); | ||
| sut.didPush(toRoute, fromRoute); | ||
| verifyInOrder([ | ||
| fixture.mockHub.generateNewTraceId(), | ||
| fixture.mockHub.startTransactionWithContext( | ||
| any, | ||
| bindToScope: anyNamed('bindToScope'), | ||
| waitForChildren: anyNamed('waitForChildren'), | ||
| autoFinishAfter: anyNamed('autoFinishAfter'), | ||
| trimEnd: anyNamed('trimEnd'), | ||
| onFinish: anyNamed('onFinish'), | ||
| customSamplingContext: anyNamed('customSamplingContext'), | ||
| startTimestamp: anyNamed('startTimestamp'), | ||
| ), | ||
| ]); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| PageRoute<dynamic> _route(RouteSettings? settings) => PageRouteBuilder<void>( | ||
| pageBuilder: (_, __, ___) => Container(), | ||
| settings: settings, | ||
| ); | ||
|
|
||
| class Fixture { | ||
| final options = defaultTestOptions(); | ||
| late final mockHub = MockHub(); | ||
| late final hub = Hub(options); | ||
|
|
||
| SentryNavigatorObserver getSut({Hub? hub}) { | ||
| hub ??= this.hub; | ||
| if (hub == mockHub) { | ||
| when(mockHub.options).thenReturn(options); | ||
| } | ||
| return SentryNavigatorObserver(hub: hub); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code doesn't make sense, we can remove it. a new start transaction with performance disabled should not be able to create a new trace, it should just no-op