Support .NET Activities #140
Conversation
lmolkova
requested review from
austinlparker,
discostu105 and
SergeyKanzhelev
as code owners
src/OpenTelemetry.Abstractions/Context/Propagation/StringExtensions.cs
Outdated
Show resolved
Hide resolved
| /// Any parent that was set previously will be discarded. | ||
| /// </summary> | ||
| /// <returns>This span builder for chaining.</returns> | ||
| ISpanBuilder FromCurrentActivity(); |
There was a problem hiding this comment.
this method ties SpanBuilder to the context. Which is very misaligned with API. Would it be better to have FromActivity method and user of API will pass Activity.Current explicitly?
There was a problem hiding this comment.
To be fair, SpanBuilder is tied to context already by using the current span unless any other parent was provided. Why do you think it is misaligned with the API?
I do not see a good scenario for FromActivity(activity) except when it is current. Moreover, I want to strictly prohibit this usage because it internally complicates things even more.
There was a problem hiding this comment.
you are right. It is tight to the current span in StartSpan method. What I don't like in this API is that it is not like any other builder methods which simply add properties. Maybe it will be more logical to create the entire SpanBuilder from the current Activity from a Tracer rather than have this method on SpanBuilder.
There was a problem hiding this comment.
Wait, we have spanbuilder to build spans, tracer does not do it and let's not abuse it.
This API simply adds property at the end of the day. It looks different than other APIs, this is right.
How about we change it to something like SetAutocollected (true) - I'll try to come up with more descriptive name, but basically this is a bool flag, that in absence of parent, creates span from current activity.
There was a problem hiding this comment.
Spent great deal of time thinking about naming and I still believe FromCurrentActivity is the most accurate one.
Still, this is naming - internally this API is no different than others - tells what context to use to create Span. I can come up with different names AssociateWithCurrentActivity / AssignToCurrentActivity()
If you have strong objections here is the alternative
SetCreateChild(true/false). It will not be compatible with any SetParent methods (as parent assumes there will be a child) and will only apply to the case when there is implicit context.- true: will create span which is child of current activity. this is default and equivalent to not calling method at all
- false: will continue current Activity without creating child
So I'll implement SetCreateChild. Let me know what you think @SergeyKanzhelev.
| /// <param name="createChild">If true, a new span will become a child of the existing implicit context. | ||
| /// If false, new span will continue this context.</param> | ||
| /// <returns>This span builder for chaining.</returns> | ||
| ISpanBuilder SetCreateChild(bool createChild); |
There was a problem hiding this comment.
this name is quite confusing. Why not move this part of deciding on whether to create one out of Activity.Current or a new one into the Tracer? This decision is only actual when you creating a SpanBuilder initially, right?
Created #145 to track it. |
* Update changelog for ESClient and MT release * fix date
Fixes #60, see explanation in #128
This change
adds SpanBuilder methods to set parent activity, or create span FOR current Activity (not a child, it's for auto-collectors that create Activity)
adds SpanBuilder/Link methods to create Link from Activity
By default, creates Span out of Activity.Current (and span attached to it)
Changes scopes to work with Activity - i.e. Activity is the propagation mechanism and Span is weakly coupled with it
All scoping happens in WithSpan call - this is where Activity.Current is changes and span is assigned to it.
Does a bunch of optimization in tracestate: Activity does not support tracestate object (it only allows strings) and more conversions of traceparent to string and back are expected in some cases. Optimizations are built around new .NET Span API (like an array pointer, not OT span)
Scenarios
Auto-collectors
Manual spans
What is NOT covered and comes next
depends on #136 - this block mergethere are some new TODO dependent on next DignosticSource preview 7 (coming in the next days) - but it shouldn't block merge