Add Probability Sampler for Activity

[pjanotti]

Fixes item 2.b on issue #684

Changes

Changed the code on the ActivityListener to handle parent context and ported the code of the existing ProbabilitySampler to work with activity, the new ProbabilisticActivitySampler following the pattern of the previous one.

Checklist

• I ran Unit Tests locally.

[pjanotti]

This is the same test code already used by ProbabilitySampler slightly modified to used the Activity equivalent types.

[cijothomas]

I am not sure if this is correct (except when starting a brand new activity without parent)
If there is an active parent (explicit or Activity.Current), then the traceid for the parent will be used as traceid for the activity to be created.

If there was no parent, then the ActivityTraceId.CreateRandom(); you pass to Sampling will be different from actual TraceId of the Activity created.

[cijothomas]

The unit test you have validates this. Can we state that TraceId is different only for the Root one scenario, and for everything else, the traceid is the correct one of the activity to be created.

[pjanotti]

Agreed: the behavior here is a bit tricky. For the ProbabilityActivitySampler itself, it works because as you said it respects the sampled flag. However, in principle, someone may want to write a sampler that does its selection based on the actual bits of the traceId, and the one received by the sampler would not be the actual traceId of the root span. In other words per OTel spec, it is possible to write a sampler that selects based on traceId bits, ignoring the sampled flag, and still sampling complete traces.

[pjanotti]

Ideally, we should be able to pass the already generated traceId to the Activity to be created.

@tarekgh I remember that you had already stated this issue in other comment threads. Seeing it concretely here makes me wonder if we could force the Activity to be created to use the same one passed to the sampler.

[tarekgh]

Sorry I am not clear about the ask here. In the current APIs, you can create the parent context and pass it to ActivitySource.StartActivity, and we'll honor this context and the new Activity will be created with the trace Id passed in this parent context. I am seeing you have full control over how you want the new Activity will be created with which trace id.

[pjanotti]

@cijothomas @tarekgh Putting this together with https://github.com/open-telemetry/opentelemetry-dotnet/pull/702/files#r431881590 I think that we should consider if all work that is done here (BuildSamplingParameters method) should be done before calling the first GetRequestedDataUsingContext directly in System.Diagnostics.

The main downside that I see is that since the current Activity is only created if sampled we may need to add it to the AsyncLocal for the "current context" otherwise this will be creating a bunch of traceIds if there are nested activities not being sampled.

[pjanotti]

Actually there is a bug with the code as it is: if the real "logical root" is not sampled it is possible that one of its "logical children" becomes sampled and it will show up as an incorrect "root". The proposal above should fix that.

[cijothomas]

https://github.com/dotnet/runtime/pull/37185/files#diff-ccf594406736f7234e01bead593d4c8aR140 - @pjanotti Please check this PR. It should address our concern?

[pjanotti]

Partially: it solves the handling of the parent in the case that the root span is sampled (see https://github.com/open-telemetry/opentelemetry-dotnet/pull/702/files#r432778065). However, it doesn't solve the case of not sampling the root/parent since Activity.Current is going to be null in such cases and the original traceId for the (not sampled) root span lost.

In order to make each step clear and easy to review I suggest the PR on dotnet/runtime (dotnet/runtime#37185) to be merged as it is since it fixes a specific issue on its own and we address the other part separately.

/cc @cijothomas @tarekgh

[cijothomas]

I wonder if .NET Activity API can take care of this part - if user did not explicitly use activitySource.StartActivity(name, parentcontext/id), can it automatically use Activity.Current?
@tarekgh please share your thoughts on this.

[tarekgh]

Can you clarify more here? The current APIs, if you pass the parent context == default, then we'll use Activity.Current as the parent for the newly created Activity. What exactly the ask here?

[cijothomas]

Tarek and I chatted offline and clarified this.

Essentially the ask is, when doing StartActivity() without explicitly passing parentContext, can ActivitySource implicitly take Activity.Current.Context.
(This is what Paulo is doing here, but the ask is to move it into .NET itself)

[tarekgh]

Thanks for your clarification. I'll look at that.

[cijothomas]

when we fit sampling into GetRequestedDataUsingParentId (we current do only GetRequestedDataUsingContext), the sampling parameter will not have ActivityContext, but only the parentid.
(just posting a note for later revisit)

[pjanotti]

Except by the TraceId this becomes ActivityCreationOptions<T> depending on how the other issues are handled we may be able to use it directly.

[cijothomas]

Looks correct.
not marking as approved now, as I'd like to discuss some comments I have left.
(also rename span to activity in comments)

[pjanotti]

PR dotnet/runtime#37185 removes the need for this part of the code.

[tarekgh]

I am not sure if you'll merge this OT PR as is till getting the .NET change. but if you do, please be aware that parentActivity.Context is possible to throw if the parent was created with defaulted trace id. my fix in .NET is taking care of this issue too to guarantee Activity.Context not throw under any condition.

[pjanotti]

Thanks for the heads up @tarekgh - I think that for now it is better to go as it is so we can keep experimenting with the ActivitySource this code will be removed as soon as we have access to dotnet/runtime#37185

[pjanotti]

@cijothomas let me know if I missed any issue that you want to address.