Adding optional parameter to facilitate resourcebuilder configuration…

[brandonh-msft]

… from caller

---

Enables usage of UseAzureMonitor() that allows for specifying a configureResource callback to configure the resource builder. This will be chained with the already-internally-implemented resource builder configuration.

Fixes #42755

[cijothomas]

is this breaking change to the API? We cannot accept breaking changes, given this is already stable.

[brandonh-msft]

I don't know what you would consider a breaking change as it relates to optional parameters; anybody using the API today would see no impact, only folks attempting to use these public methods via reflection.

[rajkumar-rangaraj]

This is a breaking change. This changes requires an arch board approval to release this API.

Importantly, this change doesn’t match the design principles of Azure Monitor Distro. I agree with Cijo that we should apply this to the upstream.

Does this help you? It might be closer to what you want to achieve. - https://github.com/Azure/azure-sdk-for-net/tree/main/sdk/monitor/Azure.Monitor.OpenTelemetry.AspNetCore#adding-custom-resource?

[brandonh-msft]

This is a breaking change. This changes requires an arch board approval to release this API.

Importantly, this change doesn’t match the design principles of Azure Monitor Distro. I agree with Cijo that we should apply this to the upstream.

Does this help you? It might be closer to what you want to achieve. - https://github.com/Azure/azure-sdk-for-net/tree/main/sdk/monitor/Azure.Monitor.OpenTelemetry.AspNetCore#adding-custom-resource?

I understand how to configure resources in otel :) that is not the issue here. Please see the bug linked; the internals of AzMon stomp over any configured resource for the logger

I will re-work the impl away from optional params so as not to be viewed as a breaking change,

[cijothomas]

I will re-work the impl away from optional params so as not to be viewed as a breaking change,

Thanks @brandonh-msft ! Appreciate your help, but please do a writeup of the proposal in the issue (after confirming there is indeed an issue - https://github.com/Azure/azure-sdk-for-net/pull/42756/files#r1528916701).
I want to make sure you are not wasting your time with actual fixes that are not going to be accepted! A write up ideally can be used to see if the direction is acceptable or not.

[rajkumar-rangaraj]

Please see the bug linked; the internals of AzMon stomp over any configured resource for the logger

If the issue is only with logs, it will be fixed with next release of OpenTelemetry SDK.

In the current OpenTelemetry .NET SDK builder.ConfigureResource(configureResource) is applied only on traces and metrics. But next version of OpenTelemetry .NET SDK will support builder.ConfigureResource(configureResource) for all 3 signals and this should fix the issue.

Ref: https://github.com/open-telemetry/opentelemetry-dotnet/blob/dd4e212d593e0ad02decc1bf22d972fa1a275618/src/OpenTelemetry/OpenTelemetryBuilderSdkExtensions.cs#L37

[cijothomas]

configuring Resource is something upstream OpenTelemetry already provides, and AzMon distro should not add anything new here.

[brandonh-msft]

you're right, but see the linked issue; the code in AzMon stomps any configured resource builder.

[cijothomas]

If AzureMonitorDistro does not respect user resource, I fully agree it needs to be fixed! (also, AzMon does not support Resource fully - only the subset from it to populate RoleName and Instance, and rest are ignored).

Maybe the issue is right in this doc itself:
https://learn.microsoft.com/en-us/azure/azure-monitor/app/opentelemetry-configuration?tabs=aspnetcore#set-the-cloud-role-name-and-the-cloud-role-instance

It is only applied to tracing alone, not for metrics, and not for logs!
It should be:

var resourceAttributes = new Dictionary<string, object> {
    { "service.name", "my-service" },
    { "service.namespace", "my-namespace" },
    { "service.instance.id", "my-instance" }};
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddOpenTelemetry().UseAzureMonitor().ConfigureResource(r => r.AddAttributes(resourceAttributes)); //applied to all 3

Please see if this is indeed just a doc issue.

[brandonh-msft]

yes and no. I should be able to run ConfigureResource whenever I want and it should be respected. In the case of AzMon, it's not if I run ConfigureResource() before UseAzureMonitor because of the code highlighted in the issue.

[cijothomas]

That should be a fully internal fix, w/o any public api changes.
Does it work for traces,metrics and broken only for logging? (I'd assume so, if at all it is broken!).

Adding new public api overload for accepting Resource (a pure otel concept), to fix a bug is not the direction I can agree.

[cijothomas]

Please see comments. We need to align with upstream for Resource, not introduce anything in azmon distro. If there is a bug in the way it is currently handled, that needs to be fixes, but in alignment with upstream otel sdk.

[brandonh-msft]

Please see comments. We need to align with upstream for Resource, not introduce anything in azmon distro. If there is a bug in the way it is currently handled, that needs to be fixes, but in alignment with upstream otel sdk.

I believe yes, there is a bug - see the linked issue. I was trying to fix in a way that mimics how resource configuration is done in the other otel overloads w/in the SDK; a resourceBuilder callback is passed and utilized accordingly.

[azure-sdk]

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Monitor.OpenTelemetry.AspNetCore