[API Proposal]: new ActivitySource.CreateActivity overload

[Wraith2]

Background and motivation

When creating an activity using ActivitySource there are currently two overloads available which allow you to pass tags which will be available to samplers.

public Activity? CreateActivity(
	string! name, 
	ActivityKind kind, 
	ActivityContext parentContext, 
	IEnumerable<KeyValuePair<string!, object?>>? tags = null, 
	IEnumerable<ActivityLink>? links = null, 
	ActivityIdFormat idFormat = ActivityIdFormat.Unknown
);

public Activity? CreateActivity(
	string! name, 
	ActivityKind kind, 
	string? parentId, 
	IEnumerable<KeyValuePair<string!, object?>>? tags = null, 
	IEnumerable<ActivityLink>? links = null, 
	ActivityIdFormat idFormat = ActivityIdFormat.Unknown
);

If you want to pass a single tag to the sampler there isn't a good way to do it. without excessive allocations.

• If you use a TagList you'll find that there is no single value constructor and that because it is a struct which contains 8 valuetype fields it is expensive to box.
• You can't stackalloc because the KeyValuePair<string, object?> type contains references.
• You can't rent an array and pass a span because ReadOnlySpan<> is a ref struct, doesn't implement enumerable and can't be boxed.
• If you rent an array and use an ArraySegment<> it'll box the array segment because it's a struct.

The easiest thing to do is a simple single value array allocation and if you want to amortize it you'll have to manage the array pooling yourself. It would be good to have a lower allocation path to create the activity in this case because the activity is being created for every incoming request.

This came up while trying to extend OpenTelemetry TelemetryHttpModule open-telemetry/opentelemetry-dotnet-contrib#1871 (comment)

API Proposal

namespace System.Diagnostics;

public class ActivitySource
{
	public Activity? CreateActivity(
		string! name, 
		ActivityKind kind, 
		ActivityContext parentContext, 
		KeyValuePair<string!, object?>? tags = default, 
		IEnumerable<ActivityLink>? links = null, 
		ActivityIdFormat idFormat = ActivityIdFormat.Unknown
	);
}

API Usage

ActivitySource source = new ActivitySource("name");
ActivityContext context = new ActivityContext();
Activity? activity = source.CreateActivity("activity", ActivityKind.Server, context, new KeyValuePair<string,object?>("tagName","tagValue"));

Alternative Designs

If it possible to provide a way to call one of the existing overloads and avoiding per call allocations which will be discarded then that may be acceptable.

Risks

No response

[dotnet-policy-service]

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

[julealgon]

Wouldn't it be possible to leverage the new params collections feature for this as well?

[Wraith2]

If that would work downstream on netfx then yes. This specific use case is from the AspNet opentelemetry code not the AspNetCore.

[cijothomas]

Wouldn't it be possible to leverage the new params collections feature for this as well?

Good point! It might be bit tricky and also need changes to ActivityCreationOptions to move to ReadOnlySpan in a back-compact way!

Separately, I think params collections would be easy to incorporate to Metrics side, where the MeterListener callbacks are all ReadOnlySpan already!

[cijothomas]

Linking to open-telemetry/opentelemetry-dotnet-contrib#2012

[cijothomas]

@Wraith2 I suggest the following too:

1. Add StartActivity also to this.
2. We need more than 1 tags! Don't have a solid number yet, but it could be 1-9 based on OTel conventions. It may not be feasible to provide that many overloads, so maybe do what Metrics did - dedicated overload for 1,2,3 and then TagList. :)

an example needing a lot of tags: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md

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

client.address
http.request.header.
http.request.method
server.address
server.port
url.path
url.query
url.scheme
user_agent.original

[tarekgh]

I don't think we need such overloads. We can have the one that takes TagList and pass it as a reference. No copying will be needed at that time. This will give the flexibility to pass whatever number of tags including single one. new TagList { {"key", "value} }

[cijothomas]

I think the 1,2,3 tag overloads is just for convenience, not for perf.
startActivity(new KVP(key,value)) instead of writing startActivity(new TagList{key,value}).

[tarekgh]

I think the 1,2,3 tag overloads is just for convenience, not for perf.

I am seeing using TagList is much simpler.

KVP	TagList
new KeyValuePair(key,value)	new TagList { {key, value } }
new KeyValuePair(key1,value1), new KeyValuePair(key2,value2)	new TagList { {key1, value1 }, {key2, value2 }}
new KeyValuePair(key1,value1), new KeyValuePair(key2,value2), new KeyValuePair(key3,value3)	new TagList { {key1, value1 }, {key2, value2 }, {key2, value2 }}

[Wraith2]

Seems reasonable. If the TagList taken as an instance into the activity rather than enumerated and the individual items copied then the boxing of the quite large TagList won't have been wasteful.

[CodeBlanch]

@tarekgh

Let's say we add overloads which accept in TagList...

public class ActivitySource
{
	public Activity? CreateActivity(
		string name, 
		ActivityKind kind, 
		ActivityContext parentContext, 
		in TagList tags = default, 
		IEnumerable<ActivityLink>? links = null, 
		ActivityIdFormat idFormat = ActivityIdFormat.Unknown
	) {}

	public Activity? StartActivity(
		string name, 
		ActivityKind kind, 
		ActivityContext parentContext, 
		in TagList tags = default, 
		IEnumerable<ActivityLink>? links = null, 
		DateTimeOffset startTime = default
	) {}
}

👍 from me so far.

But on ActivityCreationOptions<T> we have IEnumerable<KeyValuePair<string, object?>>? Tags:

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityCreationOptions.cs

Line 108 in c5e8f83

public IEnumerable<KeyValuePair<string, object?>>? Tags { get; }

Seems like we would end up with an allocation/boxing due to that. So we may need to do something there as well?

[tarekgh]

@CodeBlanch right, that is what @cijothomas mentioned in #103245 (comment). We'll look more at the details when the time come 😄