PPI-203 : deprecate context manager and implement attach/detach APIs #192
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
|
|
||
| // zone A | ||
| zoneWithContext(active).run(() { | ||
| final span = tracer.startSpan('zone-a-attached-span')..end(); |
There was a problem hiding this comment.
I would advise against shadowing the variables.
span and token are both shadowed and that certainly makes the example a little harder to read.
One way you could tackle this is to split the zone function. The other is just using more unique names.
There was a problem hiding this comment.
Fair. I can update the variable names. Though I'm wondering if I should scrap this example for something more contrived. Like pretend to do an app "setup" or "load" or something 🤷
blakeroberts-wk
force-pushed
the
attach-detach
branch
from
87ed449 to
fa08afb
Compare
…e first non-empty stack when determining the active context
…etach not detaching context when called wihtin a nested zone
blakeroberts-wk
changed the title
| {api.Context? context, api.Tracer? tracer}) async { | ||
| context ??= api.globalContextManager.active; | ||
| tracer ??= _tracerProvider.getTracer('opentelemetry-dart'); | ||
| // TODO: @Deprecated('Will be removed in v0.20.0. Use [trace] instead') |
There was a problem hiding this comment.
I'm curious about this.
I found with detatch that I was making use of the context provided to fn a lot.
There was a problem hiding this comment.
When I had these changes before the trace helper was attaching/detaching the span it created within the zone it created, I thought it wasn't very intuitive that the argument would be the context in the zone regardless of any attached/detached context. So in the case where that context was desirable, the function argument would need to be ignored and active used instead. To make this an explicit conscious choice of the consumer, we could remove the argument. That way consumers would do active or choose the zone context specifically with contextFromZone(). But this is moot now that these will return the same context.
Still, the context as an argument now becomes a third API that returns the same value. If dartlang supported function parameter covariance, it'd be less of an impact to APIs. But alas, typing the callback as Function(Context) requires consumers to do (_) {} when favoring active over manual propagation.
There was a problem hiding this comment.
I hadn't considered that I could just retrieve it from zone. Truly that didn't cross my mind.
…ic methods of Context
| /// | ||
| /// The active context is the latest attached context, if one exists, otherwise | ||
| /// the latest zone context, if one exists, otherwise the root context. | ||
| static Context get current => |
There was a problem hiding this comment.
yeah. javascript is the only implementation that uses active
| @@ -8,7 +8,7 @@ import 'package:opentelemetry/sdk.dart' | |||
| show ConsoleExporter, SimpleSpanProcessor, TracerProviderBase; | |||
|
|
|||
| mixin EventContext { | |||
| final Context context = active; | |||
| final Context context = Context.current; | |||
There was a problem hiding this comment.
I feel like this reads well where I see it througout.
There was a problem hiding this comment.
context context context 😆
|
|
||
| // zone B | ||
| zoneWithContext(context).run(() { | ||
| tracer.startSpan('zone-b-span').end(); |
There was a problem hiding this comment.
It feels like this zone-b-span should be a child of zone-a-attached-span? But the README says it is not. I suggest to use the same span names in the README.
|
closing in favor of #196 |
Which problem is this PR solving?
Fixes #185
The current context API is unnecessarily complex and does not support the attach/detach optional APIs.
In async programming, the attach/detach APIs can be incredibly handy in passing context to async processes that cannot propagate context manually (most likely due to public or otherwise unbreakable APIs). Zones can help in this case, but do not always suffice since zones might not appropriately follow code execution. Specifically, if a process includes putting an event on a stream/sink where that event is consumed by a stream listener, the zone does not propagate from where the event was placed on the stream/sink to the stream listener's callback.
Short description of the change
The current context implementation requires a modifiable concrete implementation used to decide how to propagate context. This is an adaption to OpenTelemetry JavaScript's implementation.
However, unlike JavaScript, zones are a language native feature: its type and implementation provided by the standard library. Therefore it is okay to assume that zones are available and thus simplify the context propagation story as compared to OpenTelemetry JavaScript.
A potential solution to this end would be to replace the abstract
Contextwith the concreteZoneContext. This solution was avoided becauseZoneContextpresumes that zone values are what carries the context. This means that manual propagation would be passing around the zone itself (or rather the contrived wrapper of the zone).Instead, the changes make
Contextits own immutable propagation context. This allows a zone, also an immutable propagation context, to hold both a stack of versions (due to multiple calls to attach) or a specific version (when binding a version to a zone for zone execution).tl;dr;
Contextis a stack implemented as a linked list. Cross-cutting APIs allow stacking the stack (attach/detach) on a zone, or binding a specific version (linked list node) to a zone.Zone zoneWithContext(Context context)-> binds a specific version to a fork of the current zoneContextToken attach(Context context)-> stacks a specific version to the current zoneWith the new APIs, retrieving the active context follows a hierarchy:
Starting a span is the same: a manually specified context is used if given and the active context is used otherwise. The changed part is that the active context might be an attached context.
How Has This Been Tested?
The provided example and unit tests demonstrates functionality. Additional testing will be carried out by consuming the changes in various test applications.
Checklist: