add cross-cutting context api functions #176
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
|
QA +1 changes were tested on a dart browser app with both zone and map context propagation. Examples succinctly demonstrate functionality as well |
| @Deprecated( | ||
| 'This method will be removed in 0.19.0. Use [contextWithSpanContext(globalContextManager.active, SpanContext.invalid())] instead.') | ||
| static Context get root => contextWithSpanContext( | ||
| globalContextManager.active, SpanContext.invalid()); |
There was a problem hiding this comment.
It took me a while to understand how it works, is that possible to simplify this effort and put it as a mehod for globalContextManager or ContextManager?
There was a problem hiding this comment.
The purpose of this api is to allow creating new root spans. Rather than having users reference a root context when creating a span, tracer.startSpan() will include an optional named parameter bool newRoot. Because adding that parameter to the tracer interface is a breaking change, it will need to wait until v0.19.0. For now, folks could cast their api tracer to an sdk tracer and use newRoot. Or, as the deprecated annotation suggests, manually construct a context with an invalid span context. Either way, these are short term work arounds until a more succinct solution can be introduced
| /// Call [fn] with this [MapContext] and return its result. | ||
| @experimental | ||
| R run<R>(R Function(Context context) fn) => fn(this); |
There was a problem hiding this comment.
What's the use case for this? Seems to me like it would be simpler for the caller to just run fn(this) themselves since they'd have to have a reference to the function and the context in order to call this method.
There was a problem hiding this comment.
yes. I'm still not sold on this yet, but right now it's for api completeness because our ZoneContext impl needs a way to execute a function within its private zone reference.
I'd like to come up with a paradigm so the Context api doesn't need a run method. However, this requires the consumer to have and/or use a reference to a zone. Which is almost okay except for libraries. By having consumers need to reference the zone, libraries have to worry about portability of their tracing code vs just calling the run function whether it's needed or not (whether their consumers have registered a zone context manager or are using the no op context manager)
| /// Call [fn] in this [ZoneContext]'s [Zone] and return its result. | ||
| @experimental | ||
| R run<R>(R Function(Context context) fn) => _zone.run(() => fn(this)); |
There was a problem hiding this comment.
Now that I'm seeing this here as well, should run be a part of the Context interface? Or are we holding off on that because it's experimental?
There was a problem hiding this comment.
Holding off because that'd be a breaking change. My current plan is to add the run method to the context api in the 0.19.0 release and still mark it as experimental at least for the time being 😅 I don't like it on the context api but I don't know how else to support zones and non-zones seamlessly
| /// Records a span of the given [name] for the given function with a given | ||
| /// [api.Tracer] and marks the span as errored if an exception occurs. | ||
| @experimental | ||
| Future<T> traceContext<T>(String name, Future<T> Function(api.Context) fn, |
There was a problem hiding this comment.
Okay, I think I see now why having run defined on the Context classes is useful now. Which makes me think it should be defined on the parent Context class, too.
| try { | ||
| return await context.withSpan(span).execute(fn); | ||
| // TODO: remove this check once `run` exists on context interface |
There was a problem hiding this comment.
I think this answers my question - we're avoiding adding it to the interface right now because it would be breaking to any implementers?
There was a problem hiding this comment.
Yes. Lol I should have read your entire review before responding 😆
…non-exported factory methods
|
QA +1 |
| api.traceSync('syncTrace', () { | ||
| span = api.Context.current.span as Span; | ||
| api.traceContextSync('syncTrace', (context) { | ||
| span = api.spanFromContext(context) as Span; |
There was a problem hiding this comment.
Are these as Span casts still necessary? Looks like spanFromContext should be typed to return Span
There was a problem hiding this comment.
Yes. The cast is to an internal Span type from the SDK, not the API. The SDK type is not publicly exposed
|
QA +1 CI passes, examples produce expected output, and consumed the changes into a private project that was able to accomplish context propagation via zones without using non-exported types: final tracer = otel.globalTracerProvider.getTracer('my-tracer');
final span = tracer.startSpan('my-span');
final ctx = otel.contextWithSpan(otel.globalContextManager.active, span);
otel.spanFromContext(ctx).end(); |
|
@Workiva/release-management-p |
Which problem is this PR solving?
The context API has deprecated members without appropriate replacements.
Fixes #173, #174
Short description of the change
The following functions were added to the API:
contextWithSpan,contextWithSpanContextspanContextFromContextspanFromContexttraceContext(experimental)traceSyncContext(experimental)The following functions from the API were deprecated:
tracetraceSyncThe following changes were made to implementations but not interfaces to avoid breaking changes:
api.NoopTracer.start()andsdk.Tracer.start()support an optional argumentbool newRoot. The API'sTracerinterface will include this option in the 0.19.0 release.api.ZoneContext.run()andapi.MapContext.run()methods were added. The API'sContextinterface will include this method in the 0.19.0 release.How Has This Been Tested?
The three new examples added produce expected output.
Checklist: