[AzureMonitorOpenTelemetryDistro] AddAzureMonitorOpenTelemetry extension methods and AzureMonitorOpenTelemetryOptions

[rajkumar-rangaraj]

Related to #33865

Three new public APIs for IServiceCollection extension have been added:

1. builder.Services.AddAzureMonitorOpenTelemetry()
2. builder.Services.AddAzureMonitorOpenTelemetry(AzureMonitorOpenTelemetryOptions options)
3. builder.Services.AddAzureMonitorOpenTelemetry(Action<AzureMonitorOpenTelemetryOptions> configureAzureMonitorOpenTelemetry);

Changes

• The changes includes the addition of AzureMonitorOpenTelemetryOptions to enable or disable the collection of logs, metrics, and traces.
• Added ASP.NET Core example project to demonstrate integration and telemetry collection.
• Added OpenTelemetry.Extensions.Hosting and OpenTelemetry.Instrumentation.AspNetCore package.
• The given code snippet includes example telemetry data collected by the application, such as a request and metric data with associated properties and values.

{"name":"Request","time":"2023-02-16T00:23:19.9168503Z","iKey":"00000000-0000-0000-0000-000000000000","tags":{"ai.operation.id":"c4328d0fa3de84b218dc2be782601625","ai.user.userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/110.0.0.0 Safari/537.36","ai.operation.name":"GET /","ai.location.ip":null,"ai.cloud.role":"unknown_service:Examples.AspNetCore","ai.cloud.roleInstance":"rajrang-ai","ai.internal.sdkVersion":"dotnet7.0.3:otel1.4.0-rc.4:ext1.0.0-alpha.20230215.1"},"data":{"baseType":"RequestData","baseData":{"id":"9b7836a40b2c590b","name":"GET /","duration":"00:00:00.8437928","success":true,"responseCode":"200","url":"http://localhost:5286/","properties":{"http.flavor":"1.1","_MS.ProcessedByMetricExtractors":"(Name: X,Ver:\u00271.1\u0027)"},"ver":2}}}
{"name":"Metric","time":"2023-02-16T00:24:17.4226322Z","iKey":"00000000-0000-0000-0000-000000000000","tags":{"ai.cloud.role":"unknown_service:Examples.AspNetCore","ai.cloud.roleInstance":"rajrang-ai","ai.internal.sdkVersion":"dotnet7.0.3:otel1.4.0-rc.4:ext1.0.0-alpha.20230215.1"},"data":{"baseType":"MetricData","baseData":{"metrics":[{"ns":"OpenTelemetry.Instrumentation.AspNetCore","name":"http.server.duration","value":843.7928,"count":1,"min":843.7928,"max":843.7928}],"properties":{"http.flavor":"1.1","http.method":"GET","http.route":"/","http.scheme":"http","http.status_code":"200","net.host.name":"localhost","net.host.port":"5286"},"ver":2}}}
{"name":"Metric","time":"2023-02-16T00:24:17.4226641Z","iKey":"00000000-0000-0000-0000-000000000000","tags":{"ai.cloud.role":"unknown_service:Examples.AspNetCore","ai.cloud.roleInstance":"rajrang-ai","ai.internal.sdkVersion":"dotnet7.0.3:otel1.4.0-rc.4:ext1.0.0-alpha.20230215.1"},"data":{"baseType":"MetricData","baseData":{"metrics":[{"name":"requests/duration","value":843.7928,"count":1,"min":843.7928,"max":843.7928}],"properties":{"_MS.IsAutocollected":"True","_MS.MetricId":"requests/duration","cloud/roleInstance":"rajrang-ai","cloud/roleName":"unknown_service:Examples.AspNetCore","Request.Success":"True","request/resultCode":"200"},"ver":2}}}

[cijothomas]

Services.AddAzureMonitorOpenTelemetry()
Services.AddAzureMonitorOpenTelemetry(IConfiguration config) => usage Services.AddAzureMonitorOpenTelemetry(builder.Configuration.GetSection("AzureMonitorOpenTelemetry"))
Services.AddAzureMonitorOpenTelemetry(AzureMonitorOpenTelemetryOptions options)

Could we remove the ones taking IConfiguration? Why are they needed....

[rajkumar-rangaraj]

Could we remove the ones taking IConfiguration? Why are they needed....

If someone wants to use read the value from appSettings.json or environment variable this should help. Technically, they could bind outside and pass it as a options too. I don't have a strong preference, waiting to hear more feedback on it.

[cijothomas]

In my opinion, you should remove everything from the example project that isn't relevant to the OTel. I would remove the wwwroot, Privacy, and Error pages.

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/trace/getting-started-aspnetcore use this approach to get a leaner example.

[azure-sdk]

API change check

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

Azure.Monitor.OpenTelemetry

[cijothomas]

Could we remove the ones taking IConfiguration? Why are they needed....

If someone wants to use read the value from appSettings.json or environment variable this should help. Technically, they could bind outside and pass it as a options too. I don't have a strong preference, waiting to hear more feedback on it.

I think we could retrieve it ourselves from DI...

[reyang]

Looks nice to me, I've left several suggestions but none of them are real blockers.

[samsp-msft]

Suggestions for options:

• Have a method to add identifiers for custom metrics
• Have a method to add identifiers for custom ActivitySources
  • Should both of these be available through configuration? Is this something an SRE would want to change at runtime without having to go back to the developer to change?
• Have an option for dumping traces to the console, defaults to off, but samples should set this based on Environment.IsDevelopment,. This would map to the .AddConsoleExporter() for tracing.

Edit: This PR can go in without, and add later.

[noahfalk]

Added 4 different public API in this PR. We need to select one or two from the list and discard remaining APIs

If all four were useful I'd suggest keeping all 4, I don't know why we'd limited to only having a certain number. That said I think the 2nd and 3rd have limited utility and might cause people to shoot themselves in the foot with non-standard or non-reloadable configurations. I'd suggest starting with 1+4 and then listen to user feedback to see if that is sufficient.

[rajkumar-rangaraj]

In the first beta version, we plan to include all 4 public APIs that have been proposed. However, we may choose to remove some of them after considering customer feedback and any potential supportability issues that arise.

[noahfalk]

Glad to see us putting together an easy path :)

[noahfalk]

Do we want to give users options to enable/disable metrics/logs/traces individually? ApplicationInsights SDK only has a single DisableTelemetry option as best I can tell and I'm not sure if there was much customer feedback around that or some other motivation to do things differently?

If we do start adding options to enable/disable specific subsets of telemetry then it would be good to understand a bit of a roadmap for it so we don't paint ourselves into an awkward corner. For example if we have EnableTraces first, but then later we add a List<string> of enabled trace sources because users needed more control, and then later still we decide to break the glass exposing OpenTelemetryTracerProviderBuilder for even more advanced usage it becomes unclear how these mechanisms would compose together.

[cijothomas]

ApplicationInsights SDK only has a single DisableTelemetry option as best I can tel

Application Insights has on/off for each auto-collection module.

[cijothomas]

This is something to be discussed more. I think most users should be good with the defaults. But then they can ask for additional things like - add custom meter, activitysource, other things.. We should not say "sorry not possible. you have to get rid of distro, and do everything by hand". I am hoping we can say "you can customize otel completely while retaining the distro's one-line, by registering your own Action to DI"..

For example:
AddAzMon() - adds 4 instrumentaion libraries.
User want to add a 5th one.
it should be something like

AddAzmon() - no change here.
ConfigureTracerProviderBuilder(builder.AddMyNewInstrumentation());

^ Is this your expectation too?

[rajkumar-rangaraj]

I tested it earlier, customer would be able to get additional instrumentations, custom ActivitySource, etc., Syntax is something like you explained ^.

[noahfalk]

I feel like this still leaves several questions...

1. Will users figure that plan out?
2. Will it be a common occurrence that people want to add a little extra, and when they do they will have this (IMO) odd looking configuration code?
3. Assuming they do figure it out and people start mixing and matching the different configuration APIs, how likely is it that a new user seeing this code understands what output it produces?

services.AddAzureMonitorOpenTelemetry(o =>
{
    o.EnableMetrics = false;
}
services.ConfigureMeterProviderBuilder(b => b.AddMeter("Foo"));

If I ignore what I've seen of the implementation and just guess what behavior this should have I don't know what to expect.

It is good to know what works but I also want to be a little pointed that my concern here is about does the API look nice, will it feel easy to use and be easily understandable. I am thinking of this API as the front door of Azure Monitor for .NET developers for the next decade. It will show up in samples everywhere, it will be first (and perhaps only) lines of code they write, and it will set their first impression.

Application Insights has on/off for each auto-collection module.

Can you point me at them? I didn't find them either by searching the web or looking at some of the source. Its likely I didn't guess the right terms to search for.

[cijothomas]

Can you point me at them?

https://learn.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core?tabs=netcorenew%2Cnetcore6#use-applicationinsightsserviceoptions

^ Most of them were added over time due to user demand.

[cijothomas]

Will users figure that plan out?

I do not see any reason why users have trouble figuring this out... This has been the pattern in ApplicationInsights (does not mean its a perfect solution! It certainly has a lot of ugliness as it was built over time, and had to keep back-compat, etc, but overall it feels clean IMHO.).

The overall flow is like:

1. Basic usage with all defaults. (used in hello world, pretty much the face of app insights)
   services.AddApplicationInsightsTelemetry();

• enables app insights with all defaults (auto collection modules, few initializers, sampling processor, metric extraction, livemetrics, etc.), and reads ConnectionString from IConfig.

2. User want turn on/off things - They are given knobs for most things (on/off for most common feature, not all).
3. User want to add more things like more processor, initializers - They just add it to DI, and it'll be picked up.
4. User want to customize each auto-collection module - ConfigureTelemetryModule<DependencyTrackingTelemetryModule> with an action to fully customize the auto-collection modules.

The doc for ref: https://learn.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core?tabs=netcorenew%2Cnetcore6#configure-the-application-insights-sdk

[cijothomas]

If I ignore what I've seen of the implementation and just guess what behavior this should have I don't know what to expect.

Thats fair. I can see it can be hard to predict the behavior without reading accompanying doc

[rajkumar-rangaraj]

In my view, these public API offer customers a quick and easy way to collect telemetry and send data to application insights. We could document limited customization options, such as adding new instrumentation or changing sampling percentages, etc., However, when complex configuration updates are needed for most instrumentations within the TracerProvider/MeterProvider, we should suggest that customers use the "piece meal" approach. This approach involves manually building the TracerProvider/MeterProvider with Azure Monitor Exporter using the documentation provided by OpenTelemetry .NET SDK, without utilizing any of the distro APIs.

The plan is same for .NET Worker Service, to start with we will not have any package and customer need to take a piece meal approach.

[alexdresko]

Opinion: AddAzureMonitorOpenTelemetry should have the same overloads and functionality as AddApplicationInsightsTelemetry, including the ability to use the APPLICATIONINSIGHTS_CONNECTION_STRING env var. See the docs.

Personally, I'm looking to use option #3 in the PR description here, or an env var (again, preferably APPLICATIONINSIGHTS_CONNECTION_STRING because I'm already passing that env var to my application).

[cijothomas]

Opinion: AddAzureMonitorOpenTelemetry should have the same overloads and functionality as AddApplicationInsightsTelemetry, including the ability to use the APPLICATIONINSIGHTS_CONNECTION_STRING env var. See the docs.

Personally, I'm looking to use option #3 in the PR description here, or an env var (again, preferably APPLICATIONINSIGHTS_CONNECTION_STRING because I'm already passing that env var to my application).

APPLICATIONINSIGHTS_CONNECTION_STRING would definitely be supported.

[rajkumar-rangaraj]

Opinion: AddAzureMonitorOpenTelemetry should have the same overloads and functionality as AddApplicationInsightsTelemetry, including the ability to use the APPLICATIONINSIGHTS_CONNECTION_STRING env var. See the docs.

Personally, I'm looking to use option #3 in the PR description here, or an env var (again, preferably APPLICATIONINSIGHTS_CONNECTION_STRING because I'm already passing that env var to my application).

Alternatively, you could use

builder.Services.Configure<AzureMonitorOpenTelemetryOptions>(builder.Configuration.GetSection("AzureMonitorOpenTelemetry"));

beta-1 version of this distribution will have support for APPLICATIONINSIGHTS_CONNECTION_STRING .

[rajkumar-rangaraj]

The pull request and its description have been updated following the feedback. Now, we have implementation of these three public APIs

1. builder.Services.AddAzureMonitorOpenTelemetry()
2. builder.Services.AddAzureMonitorOpenTelemetry(AzureMonitorOpenTelemetryOptions options)
3. builder.Services.AddAzureMonitorOpenTelemetry(Action<AzureMonitorOpenTelemetryOptions> configureAzureMonitorOpenTelemetry);

[alexdresko]

Since this is merged, does the nuget package get updated automatically? If not, when will this be in the nuget package?