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 static tags to specific meter and/or instruments #4316

Closed
dpk83 opened this issue Mar 20, 2023 · 31 comments
Closed

Support for adding static tags to specific meter and/or instruments #4316

dpk83 opened this issue Mar 20, 2023 · 31 comments
Labels
enhancement New feature or request needs-runtime-change Issues which likely require changes from dotnet runtime - typically DiagnosticSource package
Milestone

Comments

@dpk83
Copy link

dpk83 commented Mar 20, 2023

Feature Request

Is your feature request related to a problem?
Yes

If so, provide a concise description of the problem.

Currently, the metric agents/extension that we use is working on adding a feature to direct different metrics to different namespaces. The way this feature is being added is by means of a tag i.e. if a user wants to direct a specific metric to a non-default namespace then the instrument needs to add an additional dimension that carries the namespace to override the default.

It's not feasible for the specific instrument add/record calls to add the specific dimensions as becomes too messy to use. E.g. consider the scenario of third party library that emits metrics. As a service owner I use 10s of 3rd party libraries that emit metrics and I want to emit metrics from different libraries to different namespaces. Not all 3rd party libraries can't be modified to support the same feature so I need an ability to add some static properties for all metrics emitted by meter objects or specific instruments.

Currently .NET instrument APIs don't provide any such mechanism and OpenTelemetry metrics implementation also doesn't provide any such mechanism either.

Describe the solution you'd like:

I should be able to configure some static tags to be included for specific instruments and/or all instruments emitted from a specific meter instance, possibly at the startup configuration time.

What do you want to happen instead? What is the expected behavior?

Describe alternatives you've considered.

There are no straightforward alternatives today when using .NET Meter APIs and OpenTelemetry. The only alternative is to wrap .NET Meter APIs behind custom meter/instrument APIs, so metrics can be customized before they are fed into .NET meter

Additional Context

Add any other context about the feature request here.

@dpk83 dpk83 added the enhancement New feature or request label Mar 20, 2023
@cijothomas cijothomas added the needs-runtime-change Issues which likely require changes from dotnet runtime - typically DiagnosticSource package label Mar 20, 2023
@cijothomas
Copy link
Member

Meter can have attributes (not yet added in .NET) - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter

Will this help you achieve the goal?

@dpk83
Copy link
Author

dpk83 commented Mar 20, 2023

Meter can have attributes (not yet added in .NET) - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter
Will this help you achieve the goal?

Attributes at meter level should work for our scenario, and whatever the mechanism is should be efficient.

Will this be added to .NET or OpenTelemetry?

Do you have any timelines for when this will be available? The support for multiple namespace via additional dimension is being added by the MDM team and it won't be really complete without this feature :(

@cijothomas
Copy link
Member

cijothomas commented Mar 20, 2023

Will this be added to .NET or OpenTelemetry?

.NET. (The Meter ctor should be modified to optionally accept this). I don't think was requested to the .NET team, so no timeline. If you are looking for this to arrive this year, then we can ask .NET team for this right now, and see if they can accommodate this for .NET 8

@dpk83
Copy link
Author

dpk83 commented Mar 20, 2023

I requested this from .NET first and Noah said that OTel probably already has this feature or might be the right place to have it so circling back here.

@cijothomas
Copy link
Member

If the ask is "Support for attributes in Meter" (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter), then this should be in .NET. (Just like schema_url). This is same for Traces too. We can create issues in runtime repo for tracking right away. (it was not a top priority, as there were no asks for this and this is pretty new in spec too)

If the ask is something else, but the above is just a mechanism to achieve your goal, yes, we need to discuss more offline and post back updates here.

@wasker
Copy link

wasker commented Mar 23, 2023

Can opening issues in runtime repo be prioritized, please?

I'm surprised that static dimensions is something that is not asked often. I see that some exporters implement this feature (e.g., Geneva), but this is bizarre to me. The static dimension is a property of data emitted by the telemetry system, not the exporter - one should be able to swap out exporters and keep the same set of static dimensions attached to the meter, log, or a trace.

@wasker
Copy link

wasker commented Mar 23, 2023

Frankly, the fact that static dimensions support has dependency on .NET runtime doing something is mind boggling to me.

@cijothomas
Copy link
Member

I'm surprised that static dimensions is something that is not asked often.

This is not about static dimensions overall - those should be modelled as Resources, which is supported aready.
This issue is about adding attributes at meter level, and the spec only added that late last year : https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md#v1130-2022-09-19

@cijothomas
Copy link
Member

Frankly, the fact that static dimensions support has dependency on .NET runtime doing something is mind boggling to me.

The entire API (for tracing and metrics) is part of System.Diagnostics.DiagnosticSource package from github.com/dotnet/runtime. This repo only has the OTel SDKs. So anything changes to the API (eg: Meter, Counter etc), should come from runtime repo!

@wasker
Copy link

wasker commented Mar 23, 2023

I'm surprised that static dimensions is something that is not asked often.

This is not about static dimensions overall - those should be modelled as Resources, which is supported aready.

Do you mind pointing me to the right sample/doc for that, please?

@cijothomas
Copy link
Member

@joebeernink
Copy link

@cijothomas I was attempting to add attributes to the Resource using the .NET provider (i.e. adding the Azure Region, Stamp, dnsdomain) where the app is running in Azure) and using the AzureMonitorMetricExporter to forward to AppInsights... I see the values showing up when I use the AzureMonitorTraceExporter and use the Console Exporter, but the Metrics in the Console don't show the dimensions coming out, and I don't see them in AppInsights.

			.ConfigureResource(resource => {
				resource.AddService(DiagnosticsConfig.ServiceName);
				resource.AddAttributes(customerInstanceConfig.Values);
				}

My question with your statement above "This is not about static dimensions overall - those should be modelled as Resources, which is supported aready", if this means that the resources should be static at creation time (they are), or does this mean that only the "service_name" is currently supported?

@cijothomas
Copy link
Member

@joebeernink If resources are showing up in Console Exporter for Metrics, but not showing up in Application Insights, this indicates an issue with Application Insights, and should be reported to Application Insights repo/team.

@cijothomas
Copy link
Member

if this means that the resources should be static at creation time (they are), or does this mean that only the "service_name" is currently supported?

Yes to "resource should be known at creation time. It cannot be modified after provider is built".
No to "only the service_name" is currently supported".

@joebeernink
Copy link

joebeernink commented Mar 31, 2023

They're showing up in the Console for Traces, but not for Metrics as far as I can tell. We're attempting to use the AspNetCoreInstrumentation capabilities and haven't tried this with custom traces / metrics

[14:08:50 INF] Request finished HTTP/1.1 GET https://localhost:7186/api/permission/ - - - 401 - application/json;+charset=utf-8 1340.4469ms
Activity.TraceId: d96eaf6fad3018478e78e3861c960873
Activity.SpanId: ad665cf842de0aaf
Activity.TraceFlags: Recorded
Activity.ActivitySourceName: Microsoft.AspNetCore
Activity.DisplayName: /api/permission/
Activity.Kind: Server
Activity.StartTime: 2023-03-31T21:08:49.2057859Z
Activity.Duration: 00:00:01.3524227
Activity.Tags:
net.host.name: localhost
net.host.port: 7186
http.method: GET
http.scheme: https
http.target: /api/permission/
http.url: https://localhost:7186/api/permission/
http.flavor: 1.1
http.user_agent: PostmanRuntime/7.31.1
requestProtocol: HTTP/1.1
http.status_code: 401
Resource associated with Activity:
EnvironmentShortName: dev
RegionShortName: wus2
StampName: 01
CustomerId: joeb
CustomerInstanceName: 001
Name: dev-wus2-01-joeb-001
DnsSubDomain: joeb001

service.name: Authorization.TwinPlatform.Permission.Api
service.instance.id: 49ca5160-4846-4e48-b54a-ae5f3c8d769c

Export http.client.duration, Measures the duration of outbound HTTP requests., Unit: ms, Meter: OpenTelemetry.Instrumentation.Http/1.0.0.0
(2023-03-31T21:08:35.1224535Z, 2023-03-31T21:08:55.0715731Z] http.flavor: 1.1 http.method: GET http.scheme: https http.status_code: 200 net.peer.name: login.microsoftonline.com Histogram
Value: Sum: 898.4864 Count: 2 Min: 312.766 Max: 585.7204
(-Infinity,0]:0
(0,5]:0
(5,10]:0
(10,25]:0
(25,50]:0
(50,75]:0
(75,100]:0
(100,250]:0
(250,500]:1
(500,750]:1
(750,1000]:0
(1000,2500]:0
(2500,5000]:0
(5000,7500]:0
(7500,10000]:0
(10000,+Infinity]:0

Export http.server.duration, Measures the duration of inbound HTTP requests., Unit: ms, Meter: OpenTelemetry.Instrumentation.AspNetCore/1.0.0.0
(2023-03-31T21:08:35.1226468Z, 2023-03-31T21:08:55.0715771Z] http.flavor: 1.1 http.method: GET http.route: api/Permission http.scheme: https http.status_code: 401 net.host.name: localhost net.host.port: 7186 Histogram
Value: Sum: 1352.4227 Count: 1 Min: 1352.4227 Max: 1352.4227
(-Infinity,0]:0
(0,5]:0
(5,10]:0
(10,25]:0
(25,50]:0
(50,75]:0
(75,100]:0
(100,250]:0
(250,500]:0
(500,750]:0
(750,1000]:0
(1000,2500]:1
(2500,5000]:0
(5000,7500]:0
(7500,10000]:0
(10000,+Infinity]:0

@cijothomas
Copy link
Member

They're showing up in the Console for Traces, but not for Metrics as far as I can tell.

Please create a new issue with a minimal repro app. Use the example from here, and add the change required to show Resource is not working :https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/docs/metrics/getting-started/Program.cs

@joebeernink
Copy link

Figured it out... the values appear when the first metric is logged when the app starts, but don't repeat each time the metric reports to the console, which makes sense since they are static at app startup.

Resource associated with Metric:
EnvironmentShortName: dev
RegionShortName: wus2
StampName: 01
CustomerId: joeb
CustomerInstanceName: 001
Name: dev-wus2-01-joeb-001
DnsSubDomain: joeb001
service.name: DemoOpenTelemetry
service.instance.id: 2e27b2b2-42b4-4c91-8532-17587ca1e8f2

@dpk83
Copy link
Author

dpk83 commented Apr 4, 2023

Created an ask on dotnet runtime for this

@noahfalk
Copy link

noahfalk commented Apr 4, 2023

I requested this from .NET first and Noah said that OTel probably already has this feature or might be the right place to have it so circling back here.

I suspect I must have misunderstood something. I agree with @cijothomas that allowing .NET developers to specify default static metrics at a Meter or Instrument scope is an API I'd expect to land in the runtime on the Meter/Instrument objects. Sorry I didn't mean to lead you in a circle. I'm guessing there would also be some work in OTel to retrieve the tags from the Meter or Instrument object and pass them along to exporters.

Not all 3rd party libraries can't be modified to support the same feature so I need an ability to add some static properties for all metrics emitted by meter objects or specific instruments.

Not that I am pushing back on adding Meter/Instrument APIs for tags at all, but what happens if the 3rd party library creates a Meter and your code never has access to it? If that is a possibility then it might still be the case that the exporter, a processor, or something else in the OTel API would facilitate adding tags to measurements produced from inaccessible Meters/Instruments.

@wasker
Copy link

wasker commented Apr 4, 2023

What is the issue number on dotnet runtime?

@joebeernink
Copy link

I created a repo with a small ASP.NET Core site to test with open telemetry. It's very basic, but illustrates what we are trying to do. https://github.com/joebeernink/DemoOpenTelemetry. Would love to work with you to try out any ideas you might have. I was at Microsoft up until a month ago working in Azure, so I've got a pretty good knowledge of how devs will want to use this stuff.

@noahfalk
Copy link

noahfalk commented Apr 4, 2023

What is the issue number on dotnet runtime?

dotnet/runtime#84321

@joebeernink
Copy link

Following up on this issue. I upgraded to the 1.5.0 Beta of OpenTelemetry.Instrumentation.Http but I'm still not seeing custom dimensions come through to Application Insights for Metrics. This is really important to help split calls up by application, region, or customer to do analysis of outbound response issues. Any ideas how to fix this?

services.AddOpenTelemetry()
.WithTracing(builder => builder
.AddHttpClientInstrumentation()
.AddConsoleExporter()
.AddAzureMonitorTraceExporter(o =>
{
o.ConnectionString = appInsightsConnection;
o.Diagnostics.IsDistributedTracingEnabled = true;
}))
.WithMetrics(builder => builder
.AddHttpClientInstrumentation()
.AddConsoleExporter()
.AddAzureMonitorMetricExporter(o =>
{
o.ConnectionString = appInsightsConnection;
}))
.ConfigureResource(resource =>
{
resource.AddService(Assembly.GetEntryAssembly()?.GetName()?.Name ?? "Unknown");

				if (willowContext != null)
				{
					resource.AddAttributes(willowContext.Values);
				}
			}
			);

@reyang reyang added this to the 1.7.0 milestone Sep 27, 2023
@CodeBlanch
Copy link
Member

Relates to #3636 & #4563

@CodeBlanch CodeBlanch removed this from the 1.7.0 milestone Sep 27, 2023
@cijothomas
Copy link
Member

Following up on this issue. I upgraded to the 1.5.0 Beta of OpenTelemetry.Instrumentation.Http but I'm still not seeing custom dimensions come through to Application Insights for Metrics.

@joebeernink
Please report this issue in Application Insights repos.

@tarekgh
Copy link
Contributor

tarekgh commented Oct 25, 2023

This issue looks done. The support for ActivitySource is tracked by #3636 and dotnet/runtime#93795. Is there any reason to keep this issue open?

@cijothomas
Copy link
Member

OTel still has some work to do to make this available in OTel.

@cijothomas cijothomas added this to the 1.7.0 milestone Oct 25, 2023
@cijothomas
Copy link
Member

Tagging for upcoming milestone (1.7.0). DS changes are ready, but OTel Metric class/elsewhere needs some changes as well.

@tarekgh
Copy link
Contributor

tarekgh commented Oct 25, 2023

I thought the remaining work is tracked by #4563. anyway, I was just trying to point if there is any action needed on this issue.

@cijothomas
Copy link
Member

Sorry my bad! You are right!

@cijothomas
Copy link
Member

#4563 duplicate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-runtime-change Issues which likely require changes from dotnet runtime - typically DiagnosticSource package
Projects
None yet
Development

No branches or pull requests

8 participants