-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Support for tracestate collection in Activity of DiagnosticSource #26463
Comments
Some additional thoughts on proposed API after discussion with @adriancole:
I will update Usage section to indicate common vs. uncommon scenarios. Please comment if you have feedback |
I would like to separate the 'HTTP-like stuff' from the Activity class. Its job is to be a container that propagates things in a way useful for causality tracining. Stuff associated with HTTP conventions for encoding and parsing stuff does not really belong here. Also the W3C rules for propagating this value don't belong in activity. My knee-jerk reaction is that 'tracestate' should be just a piece of baggage (That is the whole string that encodes all the pieces just gets put into the 'traceState'). To do this well I think it is useful to have a UTF8String version of the baggage APIs (so you don't have to convert your UTF8 to strings which is nice), and I am also open to a 'Remove' API for baggage if we need it. Effectively 'traceState' just becomes one more piece of baggage, which you already have to enumerate tu support 'correlation-context'. Thus you just need to watch for 'traceState' as you process that and encode/process it specially when you see it. I do think having UTF8String (bascially Span) versions of the Baggage APIs makes sense so as to avoid lots of allocation / parsing. |
@vancem Also the concept is not http specific. Unless your comment was about lazy parsing. Than I agree that base Activity class may not be the best place for it. That's said the idea to put it in Correlation Context will require some extra parsing and memory allocations. Especially if one will need to update the value in Does it make sense with this semantics of |
@SergeyKanzhelev. Yes I know that tracestate is a list of IDs that represent transition points. But it is also just a string that needs to be propagated through user-code so that when other operations (e.g. HTTP sends) happen, knowledgeable code can update the state and send it out. In that sense FROM ACTIVITY's point of view, tracestate is just a piece of baggage (something it remembers so that other things can use it. The only thing that Activity has to do is remember it (and propagate it to all its children). Yes, something needs to update tracestate. I am just saying that we have the choice of what that is, and arguably all that logic ideally would be together. Thus I am arguing that logic that hooks request can also do the update and update the 'tracestate' key of the baggage to be the new string that will be written to output requests. Activity's job is simply to remember it. Note that Activity's baggage is actually NOT a dictionary, and in fact, my suggestion is we make it so that you pass in Span for the key and values as UTF8 blobs, and Activity pretty much simply saves a big blob of concatenated blobs. Also note that hopefully tracestate is in fact empty most of the time and tends not to be modified (this is the single vendor case). It seems like other pieces of the correlation context might be at least as likely to be updates as this piece (is there is not a strong perf reason to treat it specially, we need to make fiddling with the correlation context reasonably efficient) I am not trying to be difficult, but I do think the devil is in the details, and we are talking about very hot code paths (anything that is per-request or faster needs careful consideration). This means we want to be super lazy about parsing (especially if common scenario may be pass through, or a simple morph that could be done without exploding the data into some data structure (like a SortedDictionary). I am OK with having a string or UTF8String blob tracestate property on Activity (in general I am OK with 'well known' pieces of baggage, since it encourages type safety, discoverability. Do you want to do that? I think the more interesting/probelmatic issue is what we do about IDs. |
@vancem I'll need to think about it and prototype a little to understand the most common patterns. Two concerns I have immediately are:
I like the idea to keep tracestate in correlation context for now as a workaround. @lmolkova it may be a good idea to work you are doing in Azure Functions. At least for the short term.
I'm not trying to be pushy and see where you are coming from =) |
1. We discussed that there potentially will be no truly generic
tracers. And every tracer may just copy current traceparent to
tracestate under it's name. So tracestate is never empty. However
talking to people implementing platforms (for instance azure services) I
see a strong desire to avoid doing it. Mostly because they don't know which
vendor will consume traces in the end of the day. Actual behavior here will
contribute into what's typical pattern and what's not.
Agree about currently no generic ones, but there could be. More
importantly the lifecycle in-process is different than out of process. If
you originate a trace in-process it could be that you don't want to
serialize your state eagerly (in case you never push anything downstream).
So you could have a special case in a library where you see no state, but
you do see parent (for correlation purposes). This depends on if the api is
exposing the serialized state. Agree that if it is deserialized authority
of in-process state, then you always have state here.
|
I'm with @vancem on this one. We have to be super careful with what gets added to If baggage is not sufficient for this, we must make sure that this has a general reason of existence - independent of any specification. The W3C spec is not yet finalized and even if it will be, it's "just one spec". It might win, it might not... There might be a new spec (or a new version of this spec) in 5 years. There already are the proprietary |
Baggage has been a liability in practice due to its lack of definition (like ttl), bloat, likelihood of leaking secrets, or abstraction breaking (ex encouraging business code to use tracing apis to do business functionality). It is problems with OT baggage (including entry control, privacy etc) which led to some of the delays in correlation context, ex how to make sure these problems aren't re-introduced under a new name. Many have mentioned baggage was a bit half-baked, if we are talking about OT baggage as opposed to the original. TLDR I would steer very clear from having request scoping behavior being in any way tainted with OT behavior which in practice is propagate anything forever downstream via headers. |
.NET's I had another look at the W3C spec document. Seems like what we currently have with But I'm now confused by @SergeyKanzhelev example in this issue where he used |
I know I'm coming late to this game, but I have some questions, some specifically about how this would relate to WCF. The W3C spec only talks about how this pertains to HTTP, but in the discussion there's talk about grpc and amqp. WCF also has it's own protocols so anything that is done has to be transport agnostic. |
Motivation
W3C spec for distributed tracing context defines a new concept of
tracestate
. In order to implement W3C specification this concept should be implemented inActivity
class.Tracestate
collection should allow to make multiple types of modifications:- remove name/value pair.
- add or modify name/value pair that will be applied to every child Activity.
- add or modify name/value pair for a certain scope. For instance - just before making an outgoing call to dependent service. This modification should only be applied to this scope, not to the parent activities.
CC: @seth-capistron, @vancem, @jacpull, @lmolkova, @glennc, @davidfowl
Proposed API
Notes
ArgumentException
for incorrectly formatted values. This may be unexpected as collection is defined as string/string collection. However throwing exception is better alternative to defining custom class..Add
method will add value to the beggining of the list, not to the end as one may expect.Usage
Read tracestate key's value
This example shows how the value of
sampling-score
which is double can be read fromTraceState
.Change of tracestate should be reflected in every child activity of this activity. Supported mutations of trace state:
Update key value
Add new key-value pair
Any other values with the same name should be removed from the list.
Delete the key-value pair
The text was updated successfully, but these errors were encountered: