Draft ActivitySource Child Activity PropagationData solution

[CodeBlanch]

/cc @cijothomas @tarekgh

Here's a scratch solution for the issue we discussed earlier today wrt ActivityDataRequest.PropagationData being hard to determine when child Activity is created/sampled.

What I tried to do was reconstruct the ActivityDataRequest used by the Parent so that it can be passed into the "sampler" logic when a child is created.

Let me know what you guys think. I didn't want to go too far with it before testing the water.

The logic is basically...

• If no ActivityContext (default), ActivityDataRequest.None.
• If ActivityTraceFlags.Recorded, ActivityDataRequest.AllDataAndRecorded.
• If there is an Activity.Current and it is IsAllDataRequested, ActivityDataRequest.AllData.
• Otherwise ActivityDataRequest.PropagationData.

When this is all happening in proc, there is enough data to do it. When that state is transmitted over the wire it's going to be hard to restore all the states. In that case you know you have a context and it is or isn't Recorded. If Recorded, you are ActivityDataRequest.AllDataAndRecorded. If not recorded you could be either ActivityDataRequest.PropagationData or ActivityDataRequest.AllData. I don't think we can solve that without expanding W3C ActivityTraceFlags. Or we could also just drop ActivityDataRequest.AllData? I don't really see the value it is adding to the mix.

[tarekgh]

CC @noahfalk

Is it expected to add more stuff to the ParentActivityState in the future? I am asking to know is it worth it to add a new type for that or we can just pass the ActivityDataRequest as extra parameter to the call backs.

[tarekgh]

by the way, thanks @CodeBlanch for the proposal :-)

[CodeBlanch]

#2 would be an alternative, much bigger, approach to solving this (and other things).

[tarekgh]

I wouldn't go that far with proposal #2 just to solve this scoped issue. Now looking at this proposal #1. Looking at the 4-bullets in the description, I think this is can be detected without any APIs. right?

If no ActivityContext (default), ActivityDataRequest.None.

You'll get the Activity context in the callback and you can assume the ActivityDataRequest.None at that time.

If ActivityTraceFlags.Recorded, ActivityDataRequest.AllDataAndRecorded.

You'll get ActivityContext in the callback and you can check the ActivityTraceFlags and set ActivityDataRequest.AllDataAndRecorded.

If there is an Activity.Current and it is IsAllDataRequested, ActivityDataRequest.AllData.

You can request Activity.Current too and handle it.

If the concern here is API usability, we can clarify the documentation and specifically list these actions there. Also, we can provide a sample code showing how you can do that.
The other idea could be we provide a simple helper method that takes the ActivtyContext and returns ActivityDataRequest value can be used in sampling. wouldn't that be a much simpler solution?

[CodeBlanch]

Essentially we're asking the user to understand the intricate details of the implementation and correctly craft this logic in their callback code:

            bool isDefaultContext = context == default;

            Activity? currentActivity = Activity.Current;
            ActivityContext initializedContext = !isDefaultContext
                ? context
                : currentActivity?.Context ?? context;

            ActivityDataRequest parentDataRequested = isDefaultContext
                ? ActivityDataRequest.None
                : (initializedContext.TraceFlags & ActivityTraceFlags.Recorded) == ActivityTraceFlags.Recorded
                    ? ActivityDataRequest.AllDataAndRecorded
                    : currentActivity?.IsAllDataRequested == true
                        ? ActivityDataRequest.AllData
                        : ActivityDataRequest.PropagationData;

Could they pull it off? Yes. I managed to :) But I think it would be nicer to give them an API that takes care of this heavy lifting.

[noahfalk]

Or we could also just drop ActivityDataRequest.AllData? I don't really see the value it is adding to the mix.

It corresponds to the OT spec option "RECORD". I am not sure how you would implement the sampling spec without it?

What I tried to do was reconstruct the ActivityDataRequest used by the Parent so that it can be passed into the "sampler" logic when a child is created.

I probably missed some backstory. Assuming you had this value what does it get used for? The OT spec for sampling does not specify that samplers are intended to be provided with the sampling decision for the parent.

[CodeBlanch]

Or we could also just drop ActivityDataRequest.AllData? I don't really see the value it is adding to the mix.

It corresponds to the OT spec option "RECORD". I am not sure how you would implement the sampling spec without it?

@noahfalk You are thinking of AllDataAndRecorded. See, it's confusing :) We set Activity.ActivityTraceFlags.Recorded/Activity.Recorded for ActivityDataRequest.AllDataAndRecorded. We set Activity.AllDataRequested when ActivityDataRequest.AllData or ActivityDataRequest.AllDataAndRecorded.

When we propagate context to another dependency we only really have 3 states. No propagation at all (ActivityDataRequest.None), propagation w/ ActivityTraceFlags.Recorded = yes (ActivityDataRequest.AllDataAndRecorded), or propagation w/ ActivityTraceFlags.Recorded = no (which could mean ActivityDataRequest.PropagationData or ActivityDataRequest.AllData, there's no way to know).

I probably missed some backstory. Assuming you had this value what does it get used for? The OT spec for sampling does not specify that samplers are intended to be provided with the sampling decision for the parent.

Ya there seems to be a lot of confusion here. But it's critical! Let me try to explain.

Let's say we are writing an ASP.NET Core web service.

• A call comes in. ASP.NET Core will look for W3C trace headers. If it finds them, it will start an activity using the propagated context. If it doesn't find them, it will start a new Activity. Both will be passed to our sampler. Up to user at this point how they want to sample that guy. If parent is Recorded, they could respect that. If not, they could make their own decision. But if the parent was propagated, they should really use ActivityDataRequest.PropagationData at a minimum so context also flows to anyone downstream? In all cases, you really need to look at the parent to know what to do.
• That call moves out of ASP.NET Core into user code and our logic starts executing. Let's say we now make a Sql call. We start a new Activity for that, under the parent created by ASP.NET Core. That will run through the sampler again. The parent's sampling decision is critical to the determination we make here. If we decided to sample the parent, we should sample this Sql call. Otherwise we'll never get a consistent trace through the system for a request. If we sample every Activity independently, you'll get scattered data.
• Sql call finishes. Now we call another web service. A new Activity is created. Same story here. If that parent was Recorded, we need to pass that along to the new service. If Parent was PropagationData, we need to pass the context. If we ignore the parent, things are broken.

Help at all?

[noahfalk]

[ActivityDataRequest.AllData] corresponds to the OT spec option "RECORD". I am not sure how you would implement the sampling spec without it?

@noahfalk You are thinking of AllDataAndRecorded.

I agree its confusing, but I still believe my original statement was correct : ) The OpenTelemetry spec outlines three values that a sampler can return:

NOT_RECORD - IsRecording() == false, span will not be recorded and all events and attributes will be dropped.
RECORD - IsRecording() == true, but Sampled flag MUST NOT be set.
RECORD_AND_SAMPLED - IsRecording() == true AND Sampled flag` MUST be set.

The naming is aggravating, but OT defines Span.IsRecording() completely differently than Activity.IsRecorded. We can't change Activity.IsRecorded because that API already shipped so unless OT changes their spec it is an unavoidable naming collision:

OT Span.IsRecording() -> Activity.IsAllDataRequested
OT sampled flag       -> Activity.IsRecorded

Translating the OT spec into Activity terminology gives us:
RECORD - Activity.IsAllDataRequested == true, but Activity.IsRecorded flag MUST NOT be set.

RECORD can not match AllDataAndRecorded because that option sets Activity.IsRecorded = true, and the spec says it must be false.

Let me try to explain...

Thanks 👍 I think your objection may be with the specification itself? The specification details exactly what data should be made available to the sampler:

SpanContext of a parent Span. Typically extracted from the wire. Can be null.
TraceId of the Span to be created. It can be different from the TraceId in the SpanContext. Typically in situations when the Span to be created starts a new Trace.
Name of the Span to be created.
SpanKind
Initial set of Attributes for the Span being constructed
Collection of links that will be associated with the Span to be created. Typically useful for batch operations, see Links Between Spans.

In particular the SpanContext of the parent includes the sampling bit, but the parent's Span.IsRecording() bit is not in the list. The .NET implementation matches the functionality of the spec albeit with the adjusted naming. The parent context's IsRecorded flag (the sampling bit) is available but Activity.IsAllDataRequested (the Span.IsRecording() function) is not.

If that sounds correct then I think the best course of action is to first get the OpenTelemetry sig to change the spec, then we'll update the .NET implementation to match the updated spec. Or if I am still misunderstanding and it is an issue particular to the .NET implementation you could probably highlight that by showing how the task you want to do can be achieved in code when using some other reference implementation of the spec.

Hope that helps a bit? : )

[CodeBlanch]

@noahfalk Ah got it. ActivityDataRequest.AllData is there to satisfy the OT spec, makes sense. None of my comments are about the OT spec, though. I'm not comparing the OT spec to the .NET API. What I'm doing is trying to implement it :) Let me try to restate what I said using the spec perspective.

OT dotnet doesn't use the RECORD sampling level. It only uses RECORD_AND_SAMPLED. If we want to keep it though, sounds good to me. Maybe we'll improve the OT dotnet implementation down the line? That being said, there's still no way to propagate everything! .NET has 4 states, OT has 3, we can only propagate 3.

.NET:

None
PropagationData
AllData
AllDataAndRecorded

OT:

NOT_RECORD
RECORD
RECORD_AND_SAMPLED 

Available states during propagation:

No context.
A context.
A context w/ Recorded ActivityTraceFlags set.

I don't know how to reconcile these things. If NOT_RECORD should we still propagate? I can't tell from the spec. At the moment I'm thinking the mapping is...

None = No listener/sampler attached to ActivitySource.
PropagationData = NOT_RECORD (This is the minimum level returned by a sampler.)
AllData = RECORD (Not used at the moment by OT dotnet, but it could be.)
AllDataAndRecorded = RECORD_AND_SAMPLED 

No context = None. No listeners were in play in the calling application.
A context = PropagationData/NOT_RECORD
A context w/ Recorded ActivityTraceFlags set = AllDataAndRecorded/RECORD_AND_SAMPLED 

So AllData/RECORD has meaning in the application that was instrumented but won't propagate to downstream dependencies. That is an issue with the spec, not .NET, you are correct about that.

Anyway, @noahfalk before you reply, the comment about removing ActivityDataRequest.AllData is not the substantive part of what I'm bringing up. That is just tangential! The more important stuff is how to deal with the parent. That's what I would prefer you comment on, the OT team can squirrel away on how to map things.

Or if I am still misunderstanding and it is an issue particular to the .NET implementation you could probably highlight that by showing how the task you want to do can be achieved in code when using some other reference implementation of the spec.

More work for me you suck :D

Here's a sampler implementation using the current API and the mappings from above:

                // Sampling by ParentId. Who knows what to do here? Bueller?
                GetRequestedDataUsingParentId = (ref ActivityCreationOptions<string> options) => ActivityDataRequest.AllData,

                // Sampling by context.
                GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) =>
                {
                    ActivityDataRequest parentActivityDataRequest = DetermineParentActivityDataRequest(options.Parent);
                    if (parentActivityDataRequest != ActivityDataRequest.None)
                    {
                        // If Parent had a sampling decision, restore it on the child.
                        return parentActivityDataRequest;
                    }

                    // If a link is marked as recorded, respect that.
                    foreach (var link in options.Links ?? Array.Empty<ActivityLink>())
                    {
                        if ((link.Context.TraceFlags & ActivityTraceFlags.Recorded) == ActivityTraceFlags.Recorded)
                        {
                            return ActivityDataRequest.AllDataAndRecorded;
                        }
                    }

                    // Sample this root span using some algorithm.
                    return !ExecuteSamplingAlgorithm(options)
                        ? ActivityDataRequest.PropagationData
                        : ActivityDataRequest.AllDataAndRecorded;
                    // ActivityDataRequest.AllData not used, but it could be in the future.
                    // None never returned. Minimum level will be PropagationData.

                    static ActivityDataRequest DetermineParentActivityDataRequest(ActivityContext context)
                    {
                        bool isDefaultContext = context == default;

                        Activity currentActivity = Activity.Current;
                        ActivityContext initializedContext = !isDefaultContext
                            ? context
                            : currentActivity?.Context ?? context;

                        return isDefaultContext
                            ? ActivityDataRequest.None // If we had no context, there is no parent.
                            : (initializedContext.TraceFlags & ActivityTraceFlags.Recorded) == ActivityTraceFlags.Recorded
                                ? ActivityDataRequest.AllDataAndRecorded // If context has ActivityTraceFlags.Recorded than Parent was AllDataAndRecorded.
                                : currentActivity?.IsAllDataRequested == true
                                    ? ActivityDataRequest.AllData // If we have Activity.Current for the parent (same-process) restore AllData if it was used.
                                    : ActivityDataRequest.PropagationData; // Otherwise PropagationData.
                    }
                },

I think that will work using the current implementation of everything. More or less, no changes needed by the .NET team. But look at the DetermineParentActivityDataRequest logic. Most of that is specific to the implementation. We're really going to ask people to build that? I feel like we could make a better API. I'm not saying we have to, what is there works, I'm saying we should want to :)

This PR and issue #2 are all about just trying to make the API nicer. I think we should pass in the parent ActivityDataRequest and encapsulate the DetermineParentActivityDataRequest logic because one thing that is all over the OT sampling spec is "respect the parent" idea. It's part of the "Probability" sampler and the "ParentOrElse" sampler. It should be easy to accomplish in the API, which it is not IMO.

PS: In that sampler code above I wanted to implement the Probability logic. But in OT it is a function of TraceId which is not available in the .NET API until after sampling. So a change will be needed there if we are to implement the spec as-is?

[noahfalk]

Thanks for the sample, I think it may have highlighted the confusion! My understanding of the OT spec is that the sampling algorithm specified by the app developer (here represented by ExecuteSamplingAlgorithm) must be used for all spans, not only root spans. So I would expect the OpenTelemetry spec authors to say this sample code above is not a correct implementation of the spec and the SDK should not be trying to short-circuit invoking the sampler. @cijothomas are you able to confirm whether I am interpreting the spec correctly?

That being said, there's still no way to propagate everything! .NET has 4 states, OT has 3, we can only propagate 3

Continuing from above I don't believe the OT spec requires us to "propagate everything" in order to correctly implement the spec. If my understanding is wrong I am counting on @cijothomas or other OT folks to join in and say so : )
That aside I might be help explain regarding the 'None' option. If all ActivityListeners return the 'None' value this tells the runtime that the proposed Activity object does not need to be allocated. Activity.Current remains unchanged, no span id is generated, future activities will be completely unaware that this call to ActivitySource.StartActivity() ever occurred.

Based on my understanding of the OT spec I think this would be a simple valid implementation that doesn't make use of 'None':

NOT_RECORD -> PropagationOnly
RECORD -> AllData
RECORD_AND_SAMPLED -> AllDataAndRecorded

And this is an optional more optimized implementation that avoids creating unnecessary Activities:

NOT_RECORD -> PropagationOnly for Client or Server ActivityKind, None for Internal ActivityKind
RECORD -> AllData
RECORD_AND_SAMPLED -> AllDataAndRecorded

But in OT it is a function of TraceId which is not available in the .NET API until after sampling. So a change will be needed there if we are to implement the spec as-is?

This does seem like an oversight, or something that got lost during refactoring. We should be providing the traceId in the GetRequestedData callback even when there is no parent. @tarekgh could you take a look at this and pull me in if I can be helpful?

[CodeBlanch]

@noahfalk

So I would expect the OpenTelemetry spec authors to say this sample code above is not a correct implementation of the spec and the SDK should not be trying to short-circuit invoking the sampler.

GetRequestedDataUsingContext is the sampler, which we always invoke, for every activity/span.

Let me rewrite the code I had in the callback to simulate user's sampler of choice being plugged in. I'll show a few examples of how different OT spec samplers would look. You will see it doesn't change anything with regard to the complexity of determining the parent.

                // Sampling by ParentId. Who knows what to do here? Bueller?
                GetRequestedDataUsingParentId = (ref ActivityCreationOptions<string> options) => ActivityDataRequest.AllData,

                // Sampling by context.
                GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) => _UserSampler?.Invoke(ref options) ?? ActivityDataRequest.None;

OT AlwaysOn, _UserSampler = ExecuteAlwaysOnLogic

                private ActivityDataRequest ExecuteAlwaysOnLogic(ref ActivityCreationOptions<ActivityContext> options) => ActivityDataRequest.AllDataAndRecorded;

OT AlwaysOff, _UserSampler = ExecuteAlwaysOffLogic

                private ActivityDataRequest ExecuteAlwaysOffLogic(ref ActivityCreationOptions<ActivityContext> options) => ActivityDataRequest.PropagationData; // Maybe None, maybe determined by ActivityKind.

OT Probability, _UserSampler = ExecuteProbabilityLogic
Spec:

• The default behavior should be to trust the parent SampledFlag. However there should be configuration to change this.
• The default behavior is to apply the sampling probability only for Spans that are root spans (no parent) and Spans with remote parent. However there should be configuration to change this to "root spans only", or "all spans".

                private ActivityDataRequest ExecuteProbabilityLogic(ref ActivityCreationOptions<ActivityContext> options)
                {
                    ActivityDataRequest parentActivityDataRequest = DetermineParentActivityDataRequest(options.Parent);
                    if (parentActivityDataRequest != ActivityDataRequest.None)
                    {
                        // If Parent had a sampling decision, restore it on the child.
                        return parentActivityDataRequest;
                    }

                    // If a link is marked as recorded, respect that.
                    foreach (var link in options.Links ?? Array.Empty<ActivityLink>())
                    {
                        if ((link.Context.TraceFlags & ActivityTraceFlags.Recorded) == ActivityTraceFlags.Recorded)
                        {
                            return ActivityDataRequest.AllDataAndRecorded;
                        }
                    }

                    // Sample this root span using TraceId probability algorithm. TraceId unavailable in .NET API.
                    return !ShouldSampleBasedOnTraceIdAlgorithm(ref options)
                        ? ActivityDataRequest.PropagationData
                        : ActivityDataRequest.AllDataAndRecorded;
                    // ActivityDataRequest.AllData not used, but it could be in the future.
                    // None never returned. Minimum level will be PropagationData.

                    static ActivityDataRequest DetermineParentActivityDataRequest(ActivityContext context)
                    {
                        bool isDefaultContext = context == default;

                        Activity currentActivity = Activity.Current;
                        ActivityContext initializedContext = !isDefaultContext
                            ? context
                            : currentActivity?.Context ?? context;

                        return isDefaultContext
                            ? ActivityDataRequest.None // If we had no context, there is no parent.
                            : (initializedContext.TraceFlags & ActivityTraceFlags.Recorded) == ActivityTraceFlags.Recorded
                                ? ActivityDataRequest.AllDataAndRecorded // If context has ActivityTraceFlags.Recorded than Parent was AllDataAndRecorded.
                                : currentActivity?.IsAllDataRequested == true
                                    ? ActivityDataRequest.AllData // If we have Activity.Current for the parent (same-process) restore AllData if it was used.
                                    : ActivityDataRequest.PropagationData; // Otherwise PropagationData.
                    }
                }

OT ParentOrElse, _UserSampler = ExecuteParentOrElseLogic
Spec:

• This is a composite sampler. ParentOrElse(delegateSampler) either respects the parent span's sampling decision or delegates to delegateSampler for root spans.
• If parent exists:
  • If parent's SampledFlag is set to true returns RECORD_AND_SAMPLED
  • If parent's SampledFlag is set to false returns NOT_RECORD
• If no parent (root span) exists returns the result of the delegateSampler.

                private ActivityDataRequest ExecuteParentOrElseLogic(ref ActivityCreationOptions<ActivityContext> options)
                {
                    ActivityDataRequest parentActivityDataRequest = DetermineParentActivityDataRequest(options.Parent);
                    if (parentActivityDataRequest != ActivityDataRequest.None)
                    {
                        // If Parent had a sampling decision, restore it on the child.
                        return parentActivityDataRequest;
                    }

                    // If a link is marked as recorded, respect that.
                    foreach (var link in options.Links ?? Array.Empty<ActivityLink>())
                    {
                        if ((link.Context.TraceFlags & ActivityTraceFlags.Recorded) == ActivityTraceFlags.Recorded)
                        {
                            return ActivityDataRequest.AllDataAndRecorded;
                        }
                    }

                    // Sample this root span using a delegate provided by the user.
                    return _UserDelegateSampler(ref options);

                    static ActivityDataRequest DetermineParentActivityDataRequest(ActivityContext context)
                    {
                        bool isDefaultContext = context == default;

                        Activity currentActivity = Activity.Current;
                        ActivityContext initializedContext = !isDefaultContext
                            ? context
                            : currentActivity?.Context ?? context;

                        return isDefaultContext
                            ? ActivityDataRequest.None // If we had no context, there is no parent.
                            : (initializedContext.TraceFlags & ActivityTraceFlags.Recorded) == ActivityTraceFlags.Recorded
                                ? ActivityDataRequest.AllDataAndRecorded // If context has ActivityTraceFlags.Recorded than Parent was AllDataAndRecorded.
                                : currentActivity?.IsAllDataRequested == true
                                    ? ActivityDataRequest.AllData // If we have Activity.Current for the parent (same-process) restore AllData if it was used.
                                    : ActivityDataRequest.PropagationData; // Otherwise PropagationData.
                    }
                }

ParentOrElse & Probability samplers both need to understand the parent's state. In order to do that, they need DetermineParentActivityDataRequest which IMO should be provided by the API given it is tied so closely to the implementation.