Increase tags count in TagList for no allocation from 8 to a higher value

[dpk83]

Feature Request

TagList struct provides a way to avoid allocating memory when capturing metrics with upto 8 tags. This really helps achieve optimized paths for metric collection. While 8 tags works for close to 60-70% cases we still have quite a few cases where we the metrics are running above 8 tags.

The ask is to increase the number of tags for allocation free path from 8 to 12. We are running pretty close to 10 for some of our common metrics and we think that 12 should be a good tradeoff where it provides us a little wiggle-room while keeping the number still manageable and help most of the metric collections be allocation free.

[dpk83]

@noahfalk @tarekgh

[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

---

**Feature Request** `TagList` struct provides a way to avoid allocating memory when capturing metrics with upto `8` tags. This really helps achieve optimized paths for metric collection. While `8` tags works for close to 60-70% cases we still have quite a few cases where we the metrics are running above `8` tags. The ask is to increase the number of tags for allocation free path from `8` to `12`. We are running pretty close to `10` for some of our common metrics and we think that `12` should be a good tradeoff where it provides us a little wiggle-room while keeping the number still manageable and help most of the metric collections be allocation free.

Author:	dpk83
Assignees:	-
Labels:	`untriaged`, `area-System.Diagnostics.Activity`
Milestone:	8.0.0

[stephentoub]

Is the intent of TagList that it's invariably constructed to live on the stack and rarely if ever as part of an object that's on the heap?

[tarekgh]

Is the intent of TagList that it's invariably constructed to live on the stack and rarely if ever as part of an object that's on the heap?

It is intended to be used on the stack mostly to pass a variable list of tags to our APIs without allocation.

[stephentoub]

It is intended to be used on the stack mostly to pass a variable list of tags to our APIs without allocation.

Ok. It'd be worth measuring the impact the proposed change has. It's going to increase the amount of zero'ing required in the containing method call for all uses of TagList by another 64 bytes. If the majority of uses don't benefit, it's possible it could be a net negative. If it helps avoid a lot of allocation, it could of course be well worth it.

Just pointing out it's not "free".

[tarekgh]

When we introduced the TagList, we chose 8 as a guess for OTel common scenarios. But, we were watching if it is enough for mainstream cases. According to @dpk83 he said We are running pretty close to 10 for some of our common metrics. which suggests 8 is not good enough in the common cases. And yes, we understand it has some cost too for the initialization, but it is going to save a lot of allocation. The telemetry APIs which can use TagList usually is called more frequently.

[noahfalk]

@dpk83 - for the usage of TagList you care about, is this primarily from source generated code, or developers write the code manually, or some of both? Also are these tags primarily constant information that is the same for every measurement (example MachineName), or are they values that are varying?

I'm trying to figure out if we actually need more tags for the code we think devs should be writing, or if we are better off making improvements elsewhere to lessen the likelihood this scenario happens in the first place. For example source generated code can always do a stack allocation of exactly the right size without using TagList (devs can write it too of course, but its more complicated). Also if the tags are often constant data we'd get much better performance shifting usage towards static enrichment rather than having a bunch of unneeded tags get processed as part of aggregation.

[tarekgh]

For example source generated code can always do a stack allocation of exactly the right size without using TagList

This is not going to work on .NET Framework without work around it as we did in metrics implementation (by using thread static).

[dpk83]

Also are these tags primarily constant information that is the same for every measurement (example MachineName), or are they values that are varying?

No, constant values are added using static enrichers.

I'm trying to figure out if we actually need more tags for the code we think devs should be writing, or if we are better off making improvements elsewhere to lessen the likelihood this scenario happens in the first place.

I would want to go for later and that's what we strive for in R9 by keeping a close check on only adding the very necessary stuff, but OTel is not helping us much there. E.g. OTel's AspNetCore & Http instrumentation by default adds 7 tags and with the way they are adding support for multiple namespace is by adding another tag to the specific instrument for overriding the namespace. That just makes it 8 tags right there. So if a service owner adds event a single dimension via the runtime enricher then we are right away screwed.

Currently we aren't getting on OTel metrics auto instrumentation due to various factors, the number of auto added tags are also one of them. But I think you get the idea of where I am going given dotnet is also advertising otel for instrumentation.

Currently we are going to be adding pooling for more than 8 tags, so I added the ask to see if we could increase it and I believe 12 should be a safe enough number for us to avoid using pools as it will address majority of the cases.

[noahfalk]

@dpk83 - Are you concerned about a few specific places that R9 uses TagList in code, or are you concerned about the general case where developers use TagList outside of R9 code? I think we should optimize TagList performance for the general case usage we expect other devs to do. If we need to specialize a few sites in R9 that shouldn't be hard to do. On .NET Core you can stackalloc a Span of a size you calculate and fallback to a threadstatic or pooled KeyValuePair[] if the size is really big or you are on .NET Framework.

[dpk83]

Sounds good then. Let’s stay with 8 for now, we can reevaluate it later.

[tarekgh]

Closing per discussion outcome.

[antonfirsov]

While 8 tags works for close to 60-70% cases we still have quite a few cases where we the metrics are running above 8 tags.

I had a chat with @dpk83, and the above assumption turned out to be false. There are more than 10 tags in the majority of R9 cases. Although I don't see how would be able to fix TagList without penalizing all non-R9 use cases, I'm reopening this for us to have awareness and to have another round of discussion. This may also potentially impact #86281.

[tarekgh]

@dpk83 could you please explain more what has changed since last discussion?

There are more than 10 tags in the majority of R9 cases.

Do we know for sure this is the case? Did you do research across the R9 usage and have some numbers support that?

This may also potentially impact https://github.com/dotnet/runtime/issues/86281.

Is this a guess or for sure will more than 8 tags be used in the mainstream scenarios?

[antonfirsov]

This may also potentially impact https://github.com/dotnet/runtime/issues/86281.

Is this a guess or for sure will more than 8 tags be used in the mainstream scenarios?

Enrichment is not a mainstream scenario in a sense that users will rarely use it directly. But it's something middllewares like R9 will do out of the box and we want to make sure that minimizing allocations is possible with the API.

[dpk83]

There was some confusion. The total number of dimensions are almost always going to be much higher than 8 but lot of them are going to be static dimensions which will be handled differently and won't go through TagList. So, around 60% of the cases should still fit into 8 tags

[noahfalk]

I'd still propose that we pool the context object passed to the delegate, which transitively also pools the collection. At that point it doesn't matter if we use TagList or some other kind of list and all the allocations are eliminated. I don't (yet at least) think we have a good reason to change TagList.

[antonfirsov]

I'd still propose that we pool the context object passed to the delegate, which transitively also pools the collection.

For me the main reason to do that is the fact that it looks like we cannot go low-alloc with a value-type context and TagList.

but lot of them are going to be static dimensions which will be handled differently and won't go through TagList

@dpk83 are you sure? This code seems to feed the static and the enrichment tags to the same TagList:

https://github.com/dotnet/extensions/blob/99d8ccd0b8a0154c2f0336f5ec2e4c0a1d8729da/src/Libraries/Microsoft.Extensions.Http.Telemetry/Metering/HttpMeteringHandler.cs#L127-L154

[tarekgh]

For me the main reason to do that is the fact that it looks like we cannot go low-alloc with a value-type context and TagList.

Note, saving allocations comes with other perf cost of increasing the TagList size and perf hits for copying the struct. We need to ensure the allocations and the TagList size are balanced for the main scenarios.

[dpk83]

Earlier for R9 we used to add 4 tags by default so it had a leeway for 4 more tags from dynamic enrichment which I thought could cover about 60% of the cases. But now with .NET metric changes we actually will end up having closer to 6-8 tags already without enrichment so with dynamic enrichment there are higher chances of hitting the TagList limit of 8

[tarekgh]

@dpk83 would @noahfalk suggestion will work for R9 scenarios? or do you think TagList still needs to be expanded?

[dpk83]

Pooling is obviously the fallback if TagList can’t avoid allocation

[tarekgh]

Ok, I am closing the issue as pooling is going to be used anyway as the fallback. Please let me know if you anyone still think we need to expand the TagList for non-allocation over 8 tags.