ActivityListener API Improvement Proposal

[CodeBlanch]

I thought we could discuss here before moving to dotnet/runtime, if we decide to move forward at all.

Background and Motivation

I have been saying the ActivityListener API is confusing and hard to implement from the perspective of OpenTelemetry, here's a scratch attempt at improving it.

/cc @cijothomas @MikeGoldsmith @pjanotti @tarekgh @noahfalk

Proposed API

    public sealed class ActivitySource : IDisposable
    {
        // Only showing the signature changes, everything else is the same...
        public Activity? StartActivity(string name, ActivityKind kind, ActivityContext parentContext, Action<IDictionary<string, string?>>? populateTagsAction = null, IEnumerable<ActivityLink>? links = null, DateTimeOffset startTime = default) { return null; }
        public Activity? StartActivity(string name, ActivityKind kind, string parentId, Action<IDictionary<string, string?>>? populateTagsAction = null, IEnumerable<ActivityLink>? links = null, DateTimeOffset startTime = default) { return null; }
    }
    public enum ActivitySamplingDecision
    {
        None,
        PropagateOnly,
        Sample
    }
    public readonly struct ActivityCreationOptions
    {
        public ActivitySource Source { get; }
        public string Name { get; }
        public ActivityKind Kind { get; }
        public ActivityParentState Parent { get; }
        public IEnumerable<KeyValuePair<string, string?>>? Tags { get; }
        public IEnumerable<ActivityLink>? Links { get; }
    }
    public readonly struct ActivityParentState
    {
        public string? Id { get; }
        public ActivityContext? ActivityContext { get; }
    }
    public sealed class ActivityListener : IDisposable
    {
        public ActivityListener() { ActivitySampler = new PropagationOnlyActivitySampler(); }
        public ActivityListener(ActivitySampler activitySampler) { }
        public ActivitySampler ActivitySampler { get; }
        public Action<Activity>? ActivityStarted { get; set; }
        public Action<Activity>? ActivityStopped { get; set; }
        public Func<ActivitySource, bool>? ShouldListenTo { get; set; }
        public void Dispose() { }
    }
    public abstract class ActivitySampler
    {
        protected abstract bool RequiresTagsForSampling { get; }
        public abstract ActivitySamplingDecision ShouldSample(ref ActivityCreationOptions options);
    }
    public class PropagationOnlyActivitySampler : ActivitySampler
    {
        protected override bool RequiresTagsForSampling => false;
        public override ActivitySamplingDecision ShouldSample(ref ActivityCreationOptions options) => ActivitySamplingDecision.PropagateOnly;
    }

• I introduced the concept of an ActivitySampler. The callbacks were confusing. I figured, call a spade a spade.
• If you don't specify an ActivitySampler, we provide a default one called PropagationOnlyActivitySampler. We could also provide some other basic things OOB like Always, Never, Probability samplers?
• Today we have a gray area when calling StartActivity, do we or do we not provide tags? In order to improve that I switched it to a callback that can be used by the sampler to load tags if it needs them. ActivitySampler indicates what it needs using RequiresTagsForSampling flag. If set to true, we'll attempt to populate tags onto ActivityCreationOptions before calling the sampler.
• Replaced ActivityDataRequest with ActivitySamplingDecision and removed one of the states.
• This also solves the "should I sample external activity" thing by allowing you to do: ActivityListener.ActivitySampler.ShouldSample(...) invocation manually, if you want to.

Usage Examples

Basic usage. Should look almost exactly as it does today, only change is you aren't forced to use the callbacks. Provide a sampler or use the default one is the idea.

        public static void SimpleMethod()
        {
            ActivitySource myActivitySource = new ActivitySource("MyActivitySource");

            using ActivityListener Listener = new ActivityListener
            {
                ShouldListenTo = (activitySource) => activitySource.Name == "MyActivitySource"
            };

            using Activity activity = myActivitySource.StartActivity("MyServerOperation", ActivityKind.Server);

            if (activity?.IsAllDataRequested == true)
            {
                activity.AddTag("custom.tag", Guid.NewGuid().ToString("n"));
            }
        }

More advanced usage. This would be what OpenTelemetry does.

        public class DeferToParentOtherwiseProbabilitySampler : ActivitySampler
        {
            protected override bool RequiresTagsForSampling => false;
            public override ActivitySamplingDecision ShouldSample(ref ActivityCreationOptions options)
            {
                // If we have a parent, respect its sampling decision.
                if (options.Parent.ActivityContext.HasValue)
                {
                    return options.Parent.ActivityContext.Value.TraceFlags == ActivityTraceFlags.Recorded
                        ? ActivitySamplingDecision.Sample
                        : ActivitySamplingDecision.PropagateOnly;
                }

                // If we only have a parent Id, let's propagate.
                if (!string.IsNullOrEmpty(options.Parent.Id))
                    return ActivitySamplingDecision.PropagateOnly;

                // If we're at the root, let's determine based on probability.
                return ShouldSampleBasedOnProbability()
                    ? ActivitySamplingDecision.Sample
                    : ActivitySamplingDecision.PropagateOnly;
            }

            private bool ShouldSampleBasedOnProbability()
            {
                return true; // some real logic goes here...
            }
        }

        public static void ParentBasedSamplingMethod()
        {
            ActivitySource myActivitySource = new ActivitySource("MyActivitySource");

            using ActivityListener Listener = new ActivityListener(new DeferToParentOtherwiseProbabilitySampler())
            {
                ShouldListenTo = (activitySource) => activitySource.Name == "MyActivitySource"
            };

            using Activity activity = myActivitySource.StartActivity("MyServerOperation", ActivityKind.Server);

            if (activity?.IsAllDataRequested == true)
            {
                activity.AddTag("custom.tag", Guid.NewGuid().ToString("n"));
            }
        }

Really advanced usage. Sampling based on tag values. No known use case? Is called out in the OT spec.

        public class AdvancedSampler : ActivitySampler
        {
            protected override bool RequiresTagsForSampling => true;
            public override ActivitySamplingDecision ShouldSample(ref ActivityCreationOptions options)
            {
                return options.Tags?.Any(t => t.Key == "custom.prop" && t.Value == "1") == true
                    ? ActivitySamplingDecision.Sample
                    : ActivitySamplingDecision.PropagateOnly;
            }
        }

        public static void TagBasedSamplingMethod()
        {
            ActivitySource myActivitySource = new ActivitySource("MyActivitySource");

            using ActivityListener Listener = new ActivityListener(new AdvancedSampler())
            {
                ShouldListenTo = (activitySource) => activitySource.Name == "MyActivitySource"
            };

            using Activity activity = myActivitySource.StartActivity(
                "MyServerOperation",
                ActivityKind.Server,
                parentContext: default,
                (IDictionary<string, string?> tags) =>
                {
                    tags["custom.prop"] = "1";
                },
                null,
                default);

            if (activity?.IsAllDataRequested == true)
            {
                if (activity.Tags.Count() <= 0) // If we didn't already load tags via the callback.
                    activity.AddTag("custom.prop", "1");
                activity.AddTag("custom.tag", Guid.NewGuid().ToString("n"));
            }
        }