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

globalContextManager.active doesn't provide context of root span correctly #189

Open
dankhan-helmet opened this issue Sep 3, 2024 · 4 comments

Comments

@dankhan-helmet
Copy link

dankhan-helmet commented Sep 3, 2024

Describe the bug
When I use the traceContextSync or traceContext helper functions, the first call to globalContextManager.active presumably to get the root span if no others have been created, returns a NonRecordingSpan with an invalid parent spanId (0000000) instead of the root span with spanId ([]).

I assume the root span would be setup automatically so that there's always a starting context available? Maybe this assumption is wrong, but it's not clear from the docs/README.

The span exports to Jaegar okay (but it could just ignore a missing parent span so maybe the problem isn't being shown there, but exporting the same data to honeycomb, the problem is more obvious as they render the root span as a missing span).

If this isn't an error and just a user-error, I'd be grateful for some additional examples in the readme that discuss how to set up the root span (if required) and how to use these helper functions correctly. Thanks.

Steps to reproduce

final exporter = CollectorExporter(
      Uri.parse('https://api.honeycomb.io/v1/traces'),
      headers: {
         "x-otel-project": "myservice",
         "x-honeycomb-team": "<api-key>",
      },
    );

final tp =
      TracerProviderBase(processors: [BatchSpanProcessor(exporter)]);
  registerGlobalTracerProvider(tp);

  final cm = ZoneContextManager();
  registerGlobalContextManager(cm);

  traceContextSync<void>("My Span", (Context context) {
    spanFromContext(cm.active).setAttributes([
        Attribute.fromBoolean('test', true),
      ]);
  });

Note: even if you don't use the helper functions, it returns the same result with parent spanid as 0000000 instead of an empty spanid to indicate the root span, making me think there is no root span setup:

final span = tracer.startSpan("My Span");

try {
     // do work
} catch (e, s) {
span
  ..setStatus(StatusCode.error, e.toString())
  ..recordException(e, stackTrace: s);
  rethrow;
} finally {
  span.end();
}

What did you expect to see?
Expected that the first call to globalContextManager.active would get the root context and a valid recording span, that doesn't have an invalid parent SpanId

What did you see instead?
It returns a non-recording invalid span id span with parentId = 00000000 instead of [].

CleanShot 2024-09-03 at 12 51 49

What version and what artifacts are you using?
Artifacts: (opentelemetry-api, opentelemetry-sdk, which BatchSpanProcessor, )
Version: opentelemetry: ^0.18.6
How did you reference these artifacts? (excerpt from your pubspec.lock, pubspec.yaml, etc)

opentelemetry:
    dependency: "direct main"
    description:
      name: opentelemetry
      sha256: "7ae74cb06c079bc1c69160d84eaf3b386922af5b7b3c77e260d4dc265b1dbb07"
      url: "https://pub.dev"
    source: hosted
    version: "0.18.6"

Environment
Dart Version: Dart SDK version: 3.3.0 (stable) (Tue Feb 13 10:25:19 2024 +0000) on "macos_arm64"
OS: Flutter (Channel stable, 3.19.2, on macOS 13.6.4 22G513 darwin-arm64, locale en-NZ)

Additional context
Add any other context about the problem here.

@dankhan-helmet
Copy link
Author

dankhan-helmet commented Sep 5, 2024

To clarify the bug here, I switched back to 0.18.3, before the new contextManager code was deployed, and in which the previous recommendation was to use Context.current.span instead of contextManager.active in the new version - in doing this, the root span is setup correctly as a valid span and with the correct parent id, so looks to be a bug introduced with the new context manager functions where the root span isn't being set up correctly with the new contextManager setup.

Old code, working under 0.18.3, with valid root span:

Context.current.span.setAttributes([
        Attribute.fromString('platform.os', 'web'),
      ]);

Latest code, no longer working under 0.18.6 because the root span is missing:

spanFromContext(contextManager.active).setAttributes([
        Attribute.fromString('platform.os', 'web'),
      ]);

Screenshot of the active span parentId in 0.18.3 for comparison to my previous screenshot:

CleanShot 2024-09-05 at 18 09 18

@blakeroberts-wk
Copy link
Contributor

This needs further investigation.

W3C trace context specification suggests vendors should ignore traceparent headers with invalid span IDs (span IDs of zeros): https://www.w3.org/TR/trace-context/#parent-id.

The OpenTelemetry specification suggests that root spans can be identified by parent span IDs that are empty: https://opentelemetry.io/docs/concepts/signals/traces/#spans.

These are related specifications, but different. I think some of the otel-dart code related to non-recording spans is conforming to the W3C specification when it should be conforming to the OpenTelemetry specification.

Aside: the root context should not contain a value but the cross-cutting API for retrieving a span from context should return a non-recording span to act as an API compatible (non-null) no-op value.

@blakeroberts-wk
Copy link
Contributor

I think this can be fixed by replacing the line, https://github.com/Workiva/opentelemetry-dart/blob/master/lib/src/sdk/trace/exporters/collector_exporter.dart#L170, with:

      parentSpanId: span.parentSpanId.isValid ? span.parentSpanId.get() : [],

@dankhan-helmet
Copy link
Author

Yeah that makes sense, thanks for finding the fix point @blakeroberts-wk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants