Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dpk83 opened this issue Apr 4, 2023 · 17 comments
Closed

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

dpk83 opened this issue Apr 4, 2023 · 17 comments
Assignees
Labels
area-System.Diagnostics.Metric enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@dpk83
Copy link

dpk83 commented Apr 4, 2023

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 open-telemetry/opentelemetry-dotnet#4316

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 4, 2023
@dpk83
Copy link
Author

dpk83 commented Apr 4, 2023

@tarekgh @noahfalk

@tarekgh
Copy link
Member

tarekgh commented Apr 4, 2023

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

  • 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?
  • 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?

CC @reyang

@tarekgh tarekgh added this to the 8.0.0 milestone Apr 4, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 4, 2023
@noahfalk
Copy link
Member

noahfalk commented Apr 4, 2023

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
Copy link
Author

dpk83 commented Apr 5, 2023

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

  • Pre-aggregation doesn't have dependency on metrics pipeline as the .NET metering infra could add it automatically, but that will be less performant option.
  • Post-aggregation is better as it provides better performance, however the metrics pipeline like OpenTelemetry need to support it (which it should)

I vote for post-aggregation

@dpk83
Copy link
Author

dpk83 commented Apr 5, 2023

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

@tarekgh
Copy link
Member

tarekgh commented Apr 5, 2023

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?

@tarekgh tarekgh self-assigned this Apr 5, 2023
@dpk83
Copy link
Author

dpk83 commented Apr 5, 2023

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.

@reyang
Copy link

reyang commented Apr 5, 2023

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
Copy link
Author

dpk83 commented Apr 5, 2023

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
Copy link

reyang commented Apr 5, 2023

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
Copy link
Author

dpk83 commented Apr 5, 2023

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
Copy link

reyang commented Apr 5, 2023

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
Copy link
Author

dpk83 commented Apr 5, 2023

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
Copy link
Contributor

My thoughts...

  • Adding attributes as an immutable ctor thing seems super reasonable. It is part of the OTel Metrics API spec. It is also part of the specs for tracing and logs, commonly referred to as "instrumentation scope" (which includes some other stuff).

  • Just reading this issue, it seems like we are thinking .NET would automatically apply these things when a "record" happens? That is good in that the OTel SDK wouldn't need to do anything. But that is bad from the standpoint of following the OTLP spec/data model. There are specific places on OTLP the message for the "instrumentation scope" to go. 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 OTel SDK would need an update to known about the new API and do the right thing inside the OTLPExporter. All other exporters would also need an update to do something with them. Presumably the OTLP data model is doing this for efficiency so the same tags don't need to be transmitted on each data point, they have a higher-level spot.

/cc @alanwest

@tarekgh
Copy link
Member

tarekgh commented Apr 5, 2023

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 #84321 (comment)

@CodeBlanch
Copy link
Contributor

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
Copy link
Member

tarekgh commented May 26, 2023

This is addressed by #86567

@tarekgh tarekgh closed this as completed May 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Metric enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

6 participants