Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[AzureMonitorOpenTelemetryDistro] AddAzureMonitorOpenTelemetry extension methods and AzureMonitorOpenTelemetryOptions #34198
[AzureMonitorOpenTelemetryDistro] AddAzureMonitorOpenTelemetry extension methods and AzureMonitorOpenTelemetryOptions #34198
Changes from 10 commits
a3425d3
ab5bd20
af72a2e
67bacff
a3fcc7a
087a07f
c5d41ff
3d9a009
6c21d89
5edd8f4
4e5425f
d52fe73
28ff8b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ApplicationInsights SDK only has a single DisableTelemetry option as best I can tel
Application Insights has on/off for each auto-collection module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it earlier, customer would be able to get additional instrumentations, custom ActivitySource, etc., Syntax is something like you explained ^.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this still leaves several questions...
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
services.AddApplicationInsightsTelemetry();
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats fair. I can see it can be hard to predict the behavior without reading accompanying doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.