Samplers should be allowed to modify tracestate

[tarekgh]

open-telemetry/opentelemetry-specification#988
open-telemetry/opentelemetry-dotnet#1708

The diagnostics APIs allow sampling when creating a new Activity object using ActivitySource. The listeners of the Activity creation event using registered ActivityListener will get called when Activity creation is requested. When calling back the listener we encapsulate all Activity creation data inside the ActivityCreationOptions struct and pass it by ref to the callback.
OpenTelemetry specification now permitting the sampling to modify the trace state of the Activity we may create. The proposal here is to allow setting the trace state when doing the sampling.

Proposal

namespace System.Diagnostics
{
-    public readonly struct ActivityCreationOptions<T>
+    public struct ActivityCreationOptions<T>
    {
+        public string? TraceState { get; set; }
    }

Usage:

    listener.Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) =>
    {
        activityOptions.TraceState = "rojo=00f067aa0ba902b7";
        return ActivitySamplingResult.AllDataAndRecorded;
    };

Pros:

• Naturally exposing mutable property.
• Allow us in the future to add more mutable data to the structure.

Cons

• We must be careful to mark all old members of the struct as readonly to avoid breaking. There will be a risk of maintaining the code in the future. We can mitigate that with some tests.
• There could be some perf cost. I already measured that and seeing the perf difference is not noticeable in our case when passing the struct to the caller as a ref.

Alternative Proposal 1

namespace System.Diagnostics
{
    public readonly struct ActivityCreationOptions<T>
    {
        public string? TraceState { get; }
        public void SetTraceState(string? traceState) { ... }
    }

Usage:

    listener.Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) =>
    {
        activityOptions.SetTraceState("rojo=00f067aa0ba902b7");
        return ActivitySamplingResult.AllDataAndRecorded;
    };

Pros:

• We'll not remove the readony from the struct and we still can provide a way to mutate the struct.
• Perf shouldn't be affected.

Cons

• It will look ugly to provide a Set method on readonly type.
• We'll have to use something Unsafe.AsRef to mutate the field in the struct.
• In the future if we need to add anything else, we'll have to do it the same way.

Alternative Proposal 2

namespace System.Diagnostics
{
    public readonly struct ActivityCreationOptions<T>
    {
        public string? TraceState { get; }
        public ActivityCreationOptions<T> CreateOptionsWithTraceState(string? traceState) { ... }
    }

Usage:

    listener.Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) =>
    {
        activityOptions = activityOptions.CreateOptionsWithTraceState("rojo=00f067aa0ba902b7");
        return ActivitySamplingResult.AllDataAndRecorded;
    };

Pros:

• We'll not remove the readonly from the struct and we still can provide a way to mutate the struct.
• Perf shouldn't be affected.

Cons

• Callers need to manually reassign the ref after creating a new object for the options.
• If we need to add more mutable properties in the future, we'll have to expose more creation methods.

Alternative Proposal 3 (not desired)

namespace System.Diagnostics
{
    public class ActivityOptions
    {
        public string? TraceState { get; set; }
    }

    public readonly struct ActivityCreationOptions<T>
    {
        public ActivityOptions Options { get; }
    }

Usage:

    listener.Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) =>
    {
        activityOptions.Options.TraceState = "rojo=00f067aa0ba902b7";
        return ActivitySamplingResult.AllDataAndRecorded;
    };

Pros:

• We'll avoid the risk of removing the readonly on the struct.
• Perf shouldn't be affected.
• Allow adding more mutable values in the future. struct size will not grow more in the future.

Cons

• Callers will need to deal with one more type.
• One extra allocation will happen when users decide to set the trace state.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity
See info in area-owners.md if you want to be subscribed.

Issue Details

---

open-telemetry/opentelemetry-specification#988
open-telemetry/opentelemetry-dotnet#1708

Author:	tarekgh
Assignees:	-
Labels:	feature request, area-System.Diagnostics.Activity
Milestone:	7.0.0

[bartonjs]

If the new property is declared as get+init then the C# with syntax will allow for the easy copy-and-set, which can be written back through the ref to accomplish the current scenario.

namespace System.Diagnostics
{
    public readonly struct ActivityCreationOptions<T>
    {
+        public string? TraceState { get; init; }
    }
}

    listener.Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) =>
    {
        activityOptions = activityOptions with { TraceState = "rojo=00f067aa0ba902b7" };
        return ActivitySamplingResult.AllDataAndRecorded;
    };

[eerhardt]

There is no public ctor on this type (besides the parameterless ctor since it is a struct). I'm not sure how the with syntax will work without it.

UPDATE: So I tried it out, and apparently it works!

    public readonly struct MyStruct
    {
        public static MyStruct Create(int a) => new MyStruct(a, 0);
        
        private MyStruct(int a, int b)
        {
            A = a;
            B = b;
        }
        public int A { get; }
        public int B { get; init; }
    }

public class Program
{
    public static void Main(string[] args)
    {
        MyStruct s = MyStruct.Create(4);

        s = s with { B = 5 };

        Console.WriteLine(s.A);
        Console.WriteLine(s.B);
    }
}

compiles into:

	// MyStruct myStruct = MyStruct.Create(4);
	IL_0000: ldc.i4.4
	IL_0001: call valuetype MyBenchmarks.MyStruct MyBenchmarks.MyStruct::Create(int32)
	IL_0006: stloc.0
	// MyStruct myStruct2 = myStruct;
	IL_0007: ldloc.0
	IL_0008: stloc.1
	// myStruct2.B = 5;
	IL_0009: ldloca.s 1
	IL_000b: ldc.i4.5
	IL_000c: call instance void modreq([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) MyBenchmarks.MyStruct::set_B(int32)
	// myStruct = myStruct2;
	IL_0011: ldloc.1
	IL_0012: stloc.0
	// Console.WriteLine(myStruct.A);
	IL_0013: ldloca.s 0
	IL_0015: call instance int32 MyBenchmarks.MyStruct::get_A()
	IL_001a: call void [System.Console]System.Console::WriteLine(int32)
	// Console.WriteLine(myStruct.B);
	IL_001f: ldloca.s 0
	IL_0021: call instance int32 MyBenchmarks.MyStruct::get_B()
	IL_0026: call void [System.Console]System.Console::WriteLine(int32)

So it copies the struct, and sets the property. I tried it on .NET Framework as well.