Workiva / opentelemetry-dart

Apache License 2.0
61 stars 33 forks source link

Trace Context API Consolidation #185

Closed blakeroberts-wk closed 1 day ago

blakeroberts-wk commented 2 months ago

Is your feature request related to a problem?

The current Context implementation is as an interface where a registered ContextManager supplies the implementation. This allows a consumer (such as a library) to rely on the Context interface without needing to worry about the concrete implementation. In other words, the consumer does not need to worry about whether the Context implementation is a ZoneContext or MapContext.

However, in order to propagate context using zones, code must execute from the scope of Zone.run(). But to manually propagate context using a MapContext, no such requirement exists. As a result, the Context API has to supply a run() method to support one of the two context propagation methods.

Now, let's take a closer look at the two supported modes of context propagation: zone context propagation and map context propagation. Zone context propagation uses zones, simple enough. Map context propagation isn't really a unique propagation strategy. It is manual propagation. The Context stores its values in a map, but this has nothing to do with propagation. Rather than using a map, a zone can hold those values too. That ZoneContext can be propagated manually just the same as a MapContext.

So if a ZoneContext can do anything a MapContext can do, and a ZoneContext can propagate context automatically too, what's the point of having a MapContext?

  1. Language Support - If zones were not a native language feature (like with JavaScript), using zones would require a dependency on a third party library.
  2. Performance - If forking zones was slow or memory intensive.
  3. Unknown considerations.

(1) isn't the case since Dart supports zones natively. I haven't seen evidence to support (2). All that remains are unknowns, (3). I don't think it's wise to code against unknown considerations. To code for extensibility is good. But again, "anything [a map] can do [a zone] can do better 🎵."

Describe the solution you'd like

Simple, intuitive APIs.

Describe alternatives you've considered

final ContextKey contextKey = ContextKey();

Zone zoneWithContext(Context context) {
    return Zone.current
        .fork(zoneValues: {contextKey: context});
}

Context contextFromZone({Zone? zone}) {
    return (zone ?? Zone.current)[contextKey] as Context;
}

Span activeSpan() {
  return spanFromContext(contextFromZone());
}

Context rootContext() {
  return contextFromZone(zone: Zone.root);
}

void main() {
  final tracer = globalTracerProvider.getTracer('tracer');
  final parent = tracer.startSpan('parent')..end();
  final context = contextWithSpan(rootContext(), parent);

  zoneWithContext(context).run(() {
    // active returns the parent span
    activeSpan().addEvent('Hello, World!');

    // starting a span within this scope will automatically set its parent
    tracer.startSpan('child - automatic').end();
  });

  // manually propagating context works too
  tracer.startSpan('child - manual', context: context).end();
}

The above shows cross-cutting API additions that allow a Context to propagate via a Zone. This would allow Context to be a single concrete implementation.

We could take this even further and remove Context altogether in favor of using Zone directly in all cases. This would be achieved by replacing the current cross-cutting APIs for Context with Zone specifics:

final ContextKey spanKey = ContextKey();

Zone zoneWithSpan(Span span) {
  return Zone.current.fork(zoneValues: {
    spanKey: span,
  });
}

Zone zoneWithSpanContext(SpanContext spanContext) {
  return Zone.current.fork(zoneValues: {
    spanKey: NonRecordingSpan(spanContext),
  });
}

Span spanFromZone({Zone? zone}) {
  return (zone ?? Zone.current)[spanKey];
}

SpanContext spanContextFromZone({Zone? zone}) {
  return spanFromZone(zone: zone).spanContext;
}

The code snippet above:

Here are some example usages of this set of APIs:

void main() {
  final tracer = globalTracerProvider.getTracer('tracer');
  final parent = tracer.startSpan('parent')..end();
  final zone = zoneWithSpan(parent)
    ..run(() {
      // by default, `spanFromZone()` returns the zone's "active" span
      spanFromZone().addEvent('Hello, World!');

      // starting a span within this scope will automatically set its parent
      tracer.startSpan('child - automatic').end();
    });

  // manual propagation would work too
  tracer.startSpan('child - manual', zone: zone).end();

  // The span of a zone not in scope can be retrieved too.
  final otherSpan = spanFromZone(zone: otherZone);

  // Using the root zone will always imply an invalid non-recording span.
  // As consequence, a new root span can be started by referencing the root zone:
  tracer.startSpan('new root', zone: Zone.root).end();
  // this could be simplified by adding a `bool newRoot` parameter:
  tracer.startSpan('new root 2', newRoot: true).end();
  // this parameter would have less value compared to the current implementation:
  tracer.startSpan('new root 3',
      context: contextWithSpanContext(
          globalContextManager.active, SpanContext.invalid()));
}

Additional context

https://github.com/Workiva/opentelemetry-dart/pull/176

blakeroberts-wk commented 2 months ago

The following demonstrates the current APIs and revised APIs for automatic and manual context propagation.

Automatic Context Propagation

Current APIs:

Span parent = tracer.startSpan('parent')..end();
Context context = contextWithSpan(globalContextManager.active, parent);
(context as ZoneContext).run((_) {
  tracer.startSpan('child').end();
});

Revised APIs:

Span parent = tracer.startSpan('parent')..end();
Zone zone = zoneWithSpan(parent);
  ..run(() {
    tracer.startSpan('child').end();
  });

The brevity of this example makes the differences subtle:

  1. The Context class is removed/replaced by using the standard library Zone directly.
  2. The reference to the global ContextManager is removed. Under the proposed changes, an OTel "context" in Dart is a Zone. So, like Context, the concept of a ContextManager is superfluous.

Manual Context Propagation

Manual propagation is simplified to a lesser extent:

Current APIs:

Span parent = tracer.startSpan('parent')..end();
Context context = contextWithSpan(globalContextManager.active, parent);
tracer.startSpan('child', context: context).end();

Proposed APIs:

Span parent = tracer.startSpan('parent')..end();
Zone zone = zoneWithSpan(parent);
Span child = tracer.startSpan('child', zone: zone)..end();
evanweible-wf commented 2 months ago

I agree that based on the spec for Context, Dart Zones fit all the requirements as-is. And the spec even says:

Languages are expected to use the single, widely used Context implementation if one exists for them. In the cases where an extremely clear, pre-existing option is not available, OpenTelemetry MUST provide its own Context implementation.

If we wanted a middle ground that would allow for custom context management without having to use Dart Zones, we could still keep the abstract Context class and provide helpers to wrap a Zone so that it implements Context? Not sure if that's worth it, though. I lean towards the simpler approach of using Zones directly and we can always change course if a compelling use case appears.