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.

Sign up for GitHub

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

Fix trace and traceSync not creating root spans when newRoot is true #214

Merged
merged 1 commit into from
Mar 4, 2025
Merged

Fix trace and traceSync not creating root spans when newRoot is true #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 35 additions & 12 deletions lib/src/api/open_telemetry.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:async';
import 'package:meta/meta.dart';

import '../../api.dart' as api;
import '../../src/sdk/trace/tracer.dart' as sdk show Tracer;
import '../experimental_api.dart';
import 'propagation/noop_text_map_propagator.dart';
import 'trace/noop_tracer_provider.dart';
Expand Down Expand Up @@ -102,12 +103,23 @@ Future<T> trace<T>(String name, Future<T> Function() fn,
context ??= api.Context.current;
tracer ??= _tracerProvider.getTracer('opentelemetry-dart');

final span = tracer.startSpan(name,
// TODO: use start span option `newRoot` instead
context: newRoot ? api.Context.root : context,
attributes: spanAttributes,
kind: spanKind,
links: spanLinks);
// TODO: use start span option `newRoot` instead
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this TODO essentially been resolved now? thoughts on removing it here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The option is only on the sdk's implementation class of the api's interface which doesn't have the option. The next breaking release (0.16.0) will need to contain the change to add the new option to the interface that will allow us to remove this TODO and simplify the code down to just invoking the method on the given interface

var span;
if (tracer is sdk.Tracer) {
span = tracer.startSpan(name,
context: context,
attributes: spanAttributes,
kind: spanKind,
links: spanLinks,
newRoot: newRoot);
} else {
span = tracer.startSpan(name,
context: newRoot ? api.Context.root : context,
attributes: spanAttributes,
kind: spanKind,
links: spanLinks);
}

try {
return await Zone.current.fork().run(() {
final token = api.Context.attach(api.contextWithSpan(context!, span));
Expand Down Expand Up @@ -139,12 +151,23 @@ T traceSync<T>(String name, T Function() fn,
context ??= api.Context.current;
tracer ??= _tracerProvider.getTracer('opentelemetry-dart');

final span = tracer.startSpan(name,
// TODO: use start span option `newRoot` instead
context: newRoot ? api.Context.root : context,
attributes: spanAttributes,
kind: spanKind,
links: spanLinks);
// TODO: use start span option `newRoot` instead
var span;
if (tracer is sdk.Tracer) {
span = tracer.startSpan(name,
context: context,
attributes: spanAttributes,
kind: spanKind,
links: spanLinks,
newRoot: newRoot);
} else {
span = tracer.startSpan(name,
context: newRoot ? api.Context.root : context,
attributes: spanAttributes,
kind: spanKind,
links: spanLinks);
}

try {
return Zone.current.fork().run(() {
final token = api.Context.attach(api.contextWithSpan(context!, span));
Expand Down
26 changes: 26 additions & 0 deletions test/api/open_telemetry_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,32 @@ void main() {
}, tracer: tracer, context: contextWithSpan(Context.current, parent));
});

test('trace creates a root span', () async {
final parent = tracer.startSpan('parent')..end();
final context = contextWithSpan(Context.current, parent);
final token = Context.attach(context);

await trace('child', () async {
final child = spanFromContext(Context.current);
expect(child.parentSpanId.isValid, isFalse);
}, tracer: tracer, context: context, newRoot: true);

Context.detach(token);
});

test('traceSync creates a root span', () {
final parent = tracer.startSpan('parent')..end();
final context = contextWithSpan(Context.current, parent);
final token = Context.attach(context);

traceSync('child', () {
final child = spanFromContext(Context.current);
expect(child.parentSpanId.isValid, isFalse);
}, tracer: tracer, context: context, newRoot: true);

Context.detach(token);
});

test('trace catches, records, and rethrows exception', () async {
late sdk.Span span;
var caught = false;
Expand Down