dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15k stars 4.67k forks source link

Support for adding default tags to specific meter and/or instruments #84321

Closed dpk83 closed 1 year ago

dpk83 commented 1 year ago

Currently there is no way to add default tag to a meter and/or instrument. There are scenarios where there are a set of common tags that needs to be applied to a group of metrics (which doesn't apply to all the metrics but a group of metrics). Currently there is no way to add these tags directly to the meter or instrument and the caller needs to pass these tags for every instrument when invoking Add/Record method every time. This limits us in 2 ways

  1. Every instrument Add/Record call now need to add these tags on these instrument. Consider the cases where all the tags are static so their values don't change but they need to be attached to the metric, or a bigger subset of dimensions are static and need to be added to all the instruments created from a specific meter. (This about a third party library emitting metrics which always wants to add a set of tags to all metrics emitted by this library)
  2. One can't add a layer and inject additional default tags to instruments once an instrument invokes Add/Record call unless the caller is also implementing the metric listener as well. In the common scenario one would be using existing pipelines like OpenTelemetry metrics pipeline to collect metrics and we would need an ability to attach some default tags to certain instruments

I created an ask on the OpenTelemetry repo as well and it's clear that dotnet runtime is the right place for this feature, so creating one here. Please check the issue on OpenTelemetry repo for one of the actual scenario https://github.com/open-telemetry/opentelemetry-dotnet/issues/4316

dpk83 commented 1 year ago

@tarekgh @noahfalk

tarekgh commented 1 year ago

@dpk83 thanks for creating the issues. Some questions to know the expected behavior if we add this.

CC @reyang

noahfalk commented 1 year ago

I expect passing the tags to the Meter and Instrument creation methods. Do we need to allow the users to change/remove the tags after creating the meter and the instruments?

I'd propose if we do this we'd design for the tags to be immutable such as being specified as a constructor parameter. If the tags vary over time then it is probably preferable to either specify them as part of an individual measurement or create perhaps multiple Instrument/Meters with the same name that differ only by tags. We'd want to make sure OTel could appropriately handle the latter approach before recommending anyone do it.

When creating an instrument with tags, any call to Add/Record which doesn't take tags parameter will always use the instrument tags? or we can have situation that the instrument has tags and we want to allow calling Add/Record with no tags?

I'd propose we don't modify the tags passed to the listener and treat the static tags as an Instrument/Meter property that consumers can read whenever they want. Its likely that OTel (or other consumers) want the union of the static and dynamic tags but the efficient way to merge them is to do that post-aggregation rather than pre-aggregation. Keeping the two sets distinct will allow consumers to do that more efficient merge and keeps the runtime API very simple.

dpk83 commented 1 year ago

So, I would expect the following scenarios in this order of importance

  1. Immutable/static tags as part of Meter creation. These tags are available for all instruments created from that Meter. Implementations (like OpenTelemetry) then adds support for adding these static tags post aggregation.
  2. Immutable set of tags as part of Instrument creation. These tags are attached to the instrument pre or post-aggregation.

Regarding pre vs post aggregation

I vote for post-aggregation

dpk83 commented 1 year ago

Another requirement which I should probably create another issue is the ability to change the instrument name after instrument creation.

tarekgh commented 1 year ago

Immutable/static tags as part of Meter creation. These tags are available for all instruments created from that Meter.

Currently the instruments can access the Meter. Exposing a property on meter returning the attached tags should allow the instrument to access it.

Immutable set of tags as part of Instrument creation

I assume we will allow accessing these tags (like a property on the instrument), aggregators are free to use it the way it fits.

Another requirement which I should probably create another issue is the ability to change the instrument name after instrument creation.

Why do you need to do that? Are you thinking of recycling the instruments?

dpk83 commented 1 year ago

Another requirement which I should probably create another issue is the ability to change the instrument name after instrument creation.

Why do you need to do that? Are you thinking of recycling the instruments?

Here are a few scenarios

reyang commented 1 year ago

Another requirement which I should probably create another issue is the ability to change the instrument name after instrument creation.

Why do you need to do that? Are you thinking of recycling the instruments?

Here are a few scenarios

  • We have thousands of services today that emit metrics with metric names containing non alphanumeric characters e.g. \. Different backends apply different restrictions. E.g. Prometheus doesn't support these characters. While Geneva supports these and these services don't want to break their existing dashboard and monitors. We being the framework for these services need to provide the ability for them to smoothly migrate from one backend to other when needed without services needing to require to make code changes for all metrics. As .NET APIs currently doesn't support changing the name nor does it provide the ability for us to wrap Meter and Instrument classes we can't perform such operation centrally.
  • Another recent scenario is the .NET AspNetCore metrics that James is working on. Without the ability to change the name of the instrument we might end up having to write our own metrics auto collection for incoming/outgoing http requests. If we have the ability to change the name of the instrument than we can migrate to the .NET metrics directly without impacting any of our services.

I think we should use the OpenTelemetry Metrics SDK View which will work with all instrumentations rather than having instrumentation libraries / instrumented libraries doing this individually.

dpk83 commented 1 year ago

I think we should use the OpenTelemetry Metrics SDK View which will work with all instrumentations rather than having instrumentation libraries / instrumented libraries doing this individually.

@reyang Right, using view is the way but you need the capability to change the metric name in the view which is what I am going for here.

reyang commented 1 year ago

I think we should use the OpenTelemetry Metrics SDK View which will work with all instrumentations rather than having instrumentation libraries / instrumented libraries doing this individually.

@reyang Right, using view is the way but you need the capability to change the metric name in the view which is what I am going for here.

I'm confused, this is already working, no?

dpk83 commented 1 year ago

I'm confused, this is already working, no?

That's using reflection in OpenTelemetry and that's not for changing the name of the metrics, it just let's you update the metric name validation regex. What we need is the ability to change the metric name itself.

reyang commented 1 year ago

I'm confused, this is already working, no?

That's using reflection in OpenTelemetry and that's not for changing the name of the metrics, it just let's you update the metric name validation regex. What we need is the ability to change the metric name itself.

Have you looked at the link I posted above?

image

dpk83 commented 1 year ago

My bad, I didn't look at the URL, I assumed you pointed to the OpenTelemetry's extensibility via Views. I should stop making any assumptions. Thanks for correcting me and yes we can utilize this capability in this case.

CodeBlanch commented 1 year ago

My thoughts...

/cc @alanwest

tarekgh commented 1 year ago

If we were to do this "correctly" .NET would need to expose the attributes/instrumentation scope on Meter and probably not do anything with them.

The current thought is to do that like what @noahfalk mentioned in the comment https://github.com/dotnet/runtime/issues/84321#issuecomment-1496603005

CodeBlanch commented 1 year ago

Sorry @tarekgh I missed that! Agree with @noahfalk. More work for OTel (and others) that way, but I think better long term and allows us to be nice and compliant with OTLP spec.

tarekgh commented 1 year ago

This is addressed by https://github.com/dotnet/runtime/pull/86567