Feature for setting activity tags on creation

[rkargMsft]

Feature for adding tags at Activity creation time

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes

Adding a feature so that implementations can specify tags to add at Activity creation. This is to support using those tags when making sampling decisions as only tags provided in the CreateActivity/StartActivity are available at that time.

Description

Implementations of IHttpActivityCreationTagsFeature can determine what tags are desired to be used for sampling. It's important that this is an opt-in where the user specifies the tags to add since this work will get done for every Activity created for an incoming request and not just for the ones that eventually pass the sampling filter.

Example implementation:

public class BasicHttpActivityCreationTagsFeature : IHttpActivityCreationTagsFeature
{
    private readonly HttpContext _context;

    public BasicHttpActivityCreationTagsFeature(HttpContext context)
    {
        ArgumentNullException.ThrowIfNull(context);
        _context = context;
    }

    public IEnumerable<KeyValuePair<string, object?>>? ActivityCreationTags
    {
        get
        {
            if (_context?.Request is null)
            {
                return null;
            }

            var tags = new TagList();
            if (_context.Request.Host.HasValue)
            {
                tags.Add("server.address", _context.Request.Host.Host);
                if (_context.Request.Host.Port is not null)
                {
                    tags.Add("server.port", _context.Request.Host.Port);
                }
            }
            tags.Add("http.request.method", _context.Request.Method);
            tags.Add("url.scheme", _context.Request.Scheme);
            return tags;
        }
    }
}

Fixes #50488

[rkargMsft]

It may make sense to actually have the ActivityCreationTags property be IEnumerable<KeyValuePair<string, object?>>? which is what ActivityCreator takes as a parameter and then let the implementation decide what to return.

aspnetcore/src/Shared/Diagnostics/ActivityCreator.cs

Lines 13 to 22 in 6ab9fa6

	public static Activity? CreateFromRemote(
	ActivitySource activitySource,
	DistributedContextPropagator propagator,
	object distributedContextCarrier,
	DistributedContextPropagator.PropagatorGetterCallback propagatorGetter,
	string activityName,
	ActivityKind kind,
	IEnumerable<KeyValuePair<string, object?>>? tags,
	IEnumerable<ActivityLink>? links,
	bool diagnosticsOrLoggingEnabled)

[JamesNK]

What's the scenario where you want to set activity tags on creation? This seems like kind of niche feature and I'd prefer to see whether the problem can be solved with existing APIs.

For example, there is a feature to get the current request activity, IHttpActivityFeature, and there are many events during the corse of a HTTP request. Could you use one of those existing events, get the activity using IHttpActivityFeature and then set the tags?

[JamesNK]

This is to support using those tags when making sampling decisions as only tags provided in the CreateActivity/StartActivity are available at that time.

Is the scenario that you set a feature with tags earlier in the request. Tags could be different depending on the request.

An activity is created very early in a request so the number of places to set it would be limited. Can you give an example of this being used? Also, what does the activity source do with the tags?

[rkargMsft]

I'm specifically using YARP as a reverse proxy for a large number of services. There are calls for many different host names. I want to make tracing sampling decision for those calls and would like to have separate buckets for each hostname. For example, in a given time window, I want a minimum number of traces to always be sampled and then start doing probabilistic sampling after that.

As it is now, I can only get the base operation name (just HTTP <METHOD> at the time of sampling) and no tags. This means that if I have one very high throughput service and one very low throughput service then they're all in the same bucket for sampling (there's not host name or other data in tags yet to differentiate requests) and probabilistically, the low throughput service basically never gets sampled.

The only way to get tags to be available at the time of making the sampling decision is for them to be passed in to the CreateActivity or StartActivity call and the existing code hard codes that to tags:null.

The flow is:

• Create/Start Activity
  • Sampling logic determines if an Activity is returned or null based on only the parameters passed in on Create/Start
• If non-null (because sampling decided to keep the span) then additional tags or modifications can be made

I don't know of any other way to get information available to the sampler than to pass the tags in on Create/StartActivity.

This is extra work for every request, even those that end up not sampled, which is why it does nothing by default and would require a user to specifically opt in with their own implementation for specifically the data points they need in their particular scenario. For example, the vast majority of our services would not use this functionality; only our YARP usage would define an implementation to just add the host/server.address tag.

[rkargMsft]

The OpenTelemetry spec has this SHOULD requirement for tags available at span creation time:

The following attributes can be important for making sampling decisions and SHOULD be provided at span creation time (if provided at all):

http.request.method
server.address
server.port
url.full

It's not free to add those so making it possible but explicitly opt-in seems like a good change.

[JamesNK]

Right now tracing in ASP.NET Core doesn't follow the OTEL standard. When we do then those tags should be set immediately. It sounds like server address and port are what you want to use when making sampling decisions, right?

The performance cost shouldn't be a concern. We would only call ActivitySource if someone is listening to the activity, so people who care about perf and aren't listening won't be impacted.

[rkargMsft]

For our specific use case we would only use server.address.

Performance impact of this change would be initializing the feature if it's registered and doing the feature lookup if there's a listener:

Listener	Feature Registered	Change
No	No	-
Yes	No	Feature lookup
No	Yes	Feature initialization logic (implementation specific)
Yes	Yes	Feature initialization logic (implementation specific) + Feature lookup

[rkargMsft]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 62090 in repo dotnet/aspnetcore

[rkargMsft]

What are next steps on this to keep it from going stale?

[rkargMsft]

@JamesNK This continues to be a problem for us, preventing us from getting useful trace sampling on our YARP load balancer. I don't see any workaround we can do without this change or something else that updates how the Activity is created internally.

Is there anything else that can help with this this considered for the .NET 10 timeframe?

[ReubenBond]

Summarizing my understanding:

• Applications need trace sampling to reduce the trace volume to a manageable level,
• The sampling decision often relies on information which should be available in tags,
• ASP.NET Core sets tags using SetTags, after span creation,
• ... but sampling occurs at span creation time,
• ... so no tags are available to make a sampling decision on,
• This PR changes that by adding a new feature which lets middleware propagate tags to spans at creation time

Does that sound correct, @rkargMsft?

It sounds like a compelling scenario. This is for a service which high enough traffic volume that simple probabilistic sampling is causing high-volume endpoints from drowning out the traffic of low-volume endpoints to the point where the latter do not get sampled.

One follow-on question is, how should tags set at creation time interact with tags set after creation time (i.e, what is already there)?

[rkargMsft]

That sounds like an accurate summary.

The existing tags added after Activity creation are made with AddTag

aspnetcore/src/Shared/Diagnostics/ActivityCreator.cs

Line 79 in 45635b6

activity.AddTag(tag.Key, tag.Value);

so if there's a Feature registered that sets a tag (like server.address) then that will not get overwritten after sampling. It will remain with whatever value the Feature populated it with.

That seems like reasonable behavior as long as it's documented.

[rkargMsft]

An other option to consider could be hard coding an opinionated set of tags at Activity creation (like server.address and server.port) and always add those at Activity creation time.

The downsides of that are

• this cost gets paid by everyone, regardless of whether they use tags for sampling or not. This gets paid for every Activity, even the ones that eventually get sampled out. It's unclear what a good way to opt-out would be that is better than this proposed PR would offer (with this PR being strictly opt-in)
• this set of tags may not satisfy all use cases so an extension point may still be needed anyway

[JamesNK]

I don't want to add a new feature. I'd prefer that we followed what the standard proposes and makes the here about including some tags at creation time.

The problem is there is a lot of the standard we don't follow around traces, and I doubt we'll have time in .NET 10 to switch to following it.