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.

Sign up for GitHub

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

Jump to bottom

[AzureMonitorOpenTelemetryDistro] AddAzureMonitorOpenTelemetry extension methods and AzureMonitorOpenTelemetryOptions #34198

Merged
merged 13 commits into from
Feb 24, 2023
Merged

[AzureMonitorOpenTelemetryDistro] AddAzureMonitorOpenTelemetry extension methods and AzureMonitorOpenTelemetryOptions #34198

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a3425d3
Distro Public API
rajkumar-rangaraj Feb 15, 2023
ab5bd20
PR feedback
rajkumar-rangaraj Feb 16, 2023
af72a2e
Update public API
rajkumar-rangaraj Feb 16, 2023
67bacff
4 Public APIs
rajkumar-rangaraj Feb 16, 2023
a3fcc7a
Update sdk/monitor/Azure.Monitor.OpenTelemetry/tests/Examples.AspNetC…
TimothyMothra Feb 16, 2023
087a07f
Fix build error for demo project.
rajkumar-rangaraj Feb 16, 2023
c5d41ff
Fix error in sample
rajkumar-rangaraj Feb 17, 2023
3d9a009
Remove 461 packages
rajkumar-rangaraj Feb 17, 2023
6c21d89
4 APIs in samples.
rajkumar-rangaraj Feb 17, 2023
5edd8f4
Remove public api with parameters.
rajkumar-rangaraj Feb 17, 2023
4e5425f
PR feedback
rajkumar-rangaraj Feb 23, 2023
d52fe73
Update API
rajkumar-rangaraj Feb 23, 2023
28ff8b4
Apply connectionstring from AzureMonitorOpenTelemetryOptions to Azure…
rajkumar-rangaraj Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions eng/Packages.Data.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@

<!-- OpenTelemetry dependency approved for Azure.Monitor.OpenTelemetry.Exporter package only -->
<PackageReference Update="OpenTelemetry" Version="[1.4.0-rc.4]" Condition="'$(MSBuildProjectName)' == 'Azure.Monitor.OpenTelemetry.Exporter'" />
<PackageReference Update="OpenTelemetry.Extensions.Hosting" Version="1.4.0-rc.4" />
<PackageReference Update="OpenTelemetry.Instrumentation.AspNetCore" Version="1.0.0-rc9.13" />
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
<PackageReference Update="OpenTelemetry.Extensions.PersistentStorage" Version="1.0.0-beta.1" Condition="'$(MSBuildProjectName)' == 'Azure.Monitor.OpenTelemetry.Exporter'" />
</ItemGroup>

Expand Down
6 changes: 6 additions & 0 deletions sdk/monitor/Azure.Monitor.OpenTelemetry/Azure.Monitor.OpenTelemetry.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Dependent Projects", "Depen
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Azure.Monitor.OpenTelemetry.Exporter", "..\Azure.Monitor.OpenTelemetry.Exporter\src\Azure.Monitor.OpenTelemetry.Exporter.csproj", "{4158B8A7-971D-4A57-B4C8-5077BFD10AE6}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Examples.AspNetCore", "tests\Examples.AspNetCore\Examples.AspNetCore.csproj", "{6F094CAA-F6BB-4A9C-9D1F-1165F0758C99}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand All @@ -30,6 +32,10 @@ Global
{4158B8A7-971D-4A57-B4C8-5077BFD10AE6}.Debug|Any CPU.Build.0 = Debug|Any CPU
{4158B8A7-971D-4A57-B4C8-5077BFD10AE6}.Release|Any CPU.ActiveCfg = Release|Any CPU
{4158B8A7-971D-4A57-B4C8-5077BFD10AE6}.Release|Any CPU.Build.0 = Release|Any CPU
{6F094CAA-F6BB-4A9C-9D1F-1165F0758C99}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{6F094CAA-F6BB-4A9C-9D1F-1165F0758C99}.Debug|Any CPU.Build.0 = Debug|Any CPU
{6F094CAA-F6BB-4A9C-9D1F-1165F0758C99}.Release|Any CPU.ActiveCfg = Release|Any CPU
{6F094CAA-F6BB-4A9C-9D1F-1165F0758C99}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down
30 changes: 30 additions & 0 deletions sdk/monitor/Azure.Monitor.OpenTelemetry/api/Azure.Monitor.OpenTelemetry.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
namespace Azure.Monitor.OpenTelemetry
{
public static partial class AzureMonitorOpenTelemetryExtensions
{
public static Microsoft.Extensions.DependencyInjection.IServiceCollection AddAzureMonitorOpenTelemetry(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, Azure.Monitor.OpenTelemetry.AzureMonitorOpenTelemetryOptions? options = null) { throw null; }
public static Microsoft.Extensions.DependencyInjection.IServiceCollection AddAzureMonitorOpenTelemetry(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, Microsoft.Extensions.Configuration.IConfiguration configuration) { throw null; }
}
public partial class AzureMonitorOpenTelemetryOptions
{
public AzureMonitorOpenTelemetryOptions() { }
public string ConnectionString { get { throw null; } set { } }
public bool DisableLogs { get { throw null; } set { } }
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
public bool DisableMetrics { get { throw null; } set { } }
public bool DisableOfflineStorage { get { throw null; } set { } }
public bool DisableTraces { get { throw null; } set { } }
public Azure.Monitor.OpenTelemetry.Metrics Metrics { get { throw null; } set { } }
public string StorageDirectory { get { throw null; } set { } }
public Azure.Monitor.OpenTelemetry.Traces Traces { get { throw null; } set { } }
}
public partial class Metrics
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
{
public Metrics() { }
public bool DisableAspNetInstrumentation { get { throw null; } set { } }
}
public partial class Traces
{
public Traces() { }
public bool DisableAspNetInstrumentation { get { throw null; } set { } }
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
}
}
5 changes: 5 additions & 0 deletions sdk/monitor/Azure.Monitor.OpenTelemetry/src/Azure.Monitor.OpenTelemetry.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@
<IncludeOperationsSharedSource>true</IncludeOperationsSharedSource>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="OpenTelemetry.Extensions.Hosting" />
<PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\..\Azure.Monitor.OpenTelemetry.Exporter\src\Azure.Monitor.OpenTelemetry.Exporter.csproj" />
</ItemGroup>
Expand Down
70 changes: 70 additions & 0 deletions sdk/monitor/Azure.Monitor.OpenTelemetry/src/AzureMonitorOpenTelemetryExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using Azure.Monitor.OpenTelemetry.Exporter;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using OpenTelemetry.Metrics;
using OpenTelemetry.Trace;

namespace Azure.Monitor.OpenTelemetry
{
/// <summary>
/// Extension methods for setting up Azure Monitor OpenTelemetry in an <see cref="IServiceCollection" />.
/// </summary>
public static class AzureMonitorOpenTelemetryExtensions
{
/// <summary>
/// Adds Azure Monitor OpenTelemetry into service collection.
/// </summary>
/// <param name="services"><see cref="IServiceCollection"/>.</param>
/// <param name="configuration"><see cref="IConfiguration"/>.</param>
/// <returns><see cref="IServiceCollection"/></returns>
public static IServiceCollection AddAzureMonitorOpenTelemetry(this IServiceCollection services, IConfiguration configuration)
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
{
var options = new AzureMonitorOpenTelemetryOptions();
configuration.Bind(options);
return services.AddAzureMonitorOpenTelemetry(options);
}

/// <summary>
/// Adds Azure Monitor OpenTelemetry into service collection.
/// </summary>
/// <param name="services"><see cref="IServiceCollection"/>.</param>
/// <param name="options">The <see cref="AzureMonitorOpenTelemetryOptions" /> instance for configuration.</param>
/// <returns><see cref="IServiceCollection"/></returns>
public static IServiceCollection AddAzureMonitorOpenTelemetry(this IServiceCollection services, AzureMonitorOpenTelemetryOptions? options = null)
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
{
options ??= new AzureMonitorOpenTelemetryOptions();
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved

var builder = services.AddOpenTelemetry();

if (options.EnableTraces)
{
builder.WithTracing(b => b
.AddAspNetCoreInstrumentation()
.AddAzureMonitorTraceExporter(o =>
{
o.ConnectionString = options.ConnectionString;
o.DisableOfflineStorage = options.DisableOfflineStorage;
o.StorageDirectory = options.StorageDirectory;
}));
}

if (options.EnableMetrics)
{
builder.WithMetrics(b => b
.AddAspNetCoreInstrumentation()
.AddAzureMonitorMetricExporter(o =>
{
o.ConnectionString = options.ConnectionString;
o.DisableOfflineStorage = options.DisableOfflineStorage;
o.StorageDirectory = options.StorageDirectory;
}));
}

return services;
}
}
}
49 changes: 49 additions & 0 deletions sdk/monitor/Azure.Monitor.OpenTelemetry/src/AzureMonitorOpenTelemetryOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#nullable disable
TimothyMothra marked this conversation as resolved.
Show resolved Hide resolved
#pragma warning disable SA1402 // File may only contain a single type
#pragma warning disable AZC0012 // Avoid single word type names
#pragma warning disable CA1724 // Conflict Metrics name with OpenTelemetry.Metrics

namespace Azure.Monitor.OpenTelemetry
{
/// <summary>
/// Options that allow users to configure the Azure Monitor OpenTelemetry.
/// </summary>
public class AzureMonitorOpenTelemetryOptions
{
/// <summary>
/// The Connection String provides users with a single configuration setting to identify the Azure Monitor resource and endpoint.
/// </summary>
/// <remarks>
/// (https://docs.microsoft.com/azure/azure-monitor/app/sdk-connection-string).
/// </remarks>
public string ConnectionString { get; set; } = "InstrumentationKey=00000000-0000-0000-0000-000000000000";

/// <summary>
/// Gets or sets a value indicating whether Traces should be enabled.
/// </summary>
public bool EnableTraces { get; set; } = true;
Copy link

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.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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 ^.

Copy link

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...

  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.

Copy link
Contributor

@cijothomas cijothomas Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
  1. User want turn on/off things - They are given knobs for most things (on/off for most common feature, not all).
  2. User want to add more things like more processor, initializers - They just add it to DI, and it'll be picked up.
  3. 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

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.


/// <summary>
/// Gets or sets a value indicating whether Metrics should be enabled.
/// </summary>
public bool EnableMetrics { get; set; } = true;

/// <summary>
/// Gets or sets a value indicating whether Logs should be enabled.
/// </summary>
public bool EnableLogs { get; set; } = true;

/// <summary>
/// Override the default directory for offline storage.
/// </summary>
public string StorageDirectory { get; set; }

/// <summary>
/// Disable offline storage.
/// </summary>
public bool DisableOfflineStorage { get; set; }
}
}
13 changes: 13 additions & 0 deletions sdk/monitor/Azure.Monitor.OpenTelemetry/tests/Examples.AspNetCore/Examples.AspNetCore.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
<TargetFrameworks>net7.0</TargetFrameworks>
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
<Nullable>enable</Nullable>
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
<ImplicitUsings>enable</ImplicitUsings>
</PropertyGroup>

<ItemGroup>
<ProjectReference Include="..\..\src\Azure.Monitor.OpenTelemetry.csproj" />
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
</ItemGroup>

</Project>
34 changes: 34 additions & 0 deletions sdk/monitor/Azure.Monitor.OpenTelemetry/tests/Examples.AspNetCore/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using System.Diagnostics;
using Azure.Monitor.OpenTelemetry;
using OpenTelemetry.Instrumentation.AspNetCore;

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.
builder.Services.AddControllersWithViews();

builder.Services.Configure<AspNetCoreInstrumentationOptions>(o =>
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
{
o.EnrichWithHttpRequest = (activity, httpRequest) =>
{
activity.SetTag("requestProtocol", httpRequest.Protocol);
};
o.EnrichWithHttpResponse = (activity, httpResponse) =>
{
activity.SetTag("responseLength", httpResponse.ContentLength);
};
o.EnrichWithException = (activity, exception) =>
{
activity.SetTag("exceptionType", exception.GetType().ToString());
};
});

builder.Services.AddAzureMonitorOpenTelemetry();

// This is another overload to call AddAzureMonitorOpenTelemetry with IConfiguration.
// builder.Services.AddAzureMonitorOpenTelemetry(builder.Configuration.GetSection("AzureMonitorOpenTelemetry"));

var app = builder.Build();
app.MapGet("/", () => $"Hello World! OpenTelemetry Trace: {Activity.Current?.Id}");

app.Run();
8 changes: 8 additions & 0 deletions ...onitor/Azure.Monitor.OpenTelemetry/tests/Examples.AspNetCore/appsettings.Development.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"Logging": {
"LogLevel": {
"Default": "Information",
"Microsoft.AspNetCore": "Warning"
}
}
}
17 changes: 17 additions & 0 deletions sdk/monitor/Azure.Monitor.OpenTelemetry/tests/Examples.AspNetCore/appsettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"AzureMonitorOpenTelemetry": {
"ConnectionString": "InstrumentationKey=00000000-0000-0000-0000-000000000000",
"DisableTraces": false,
"DisableMetrics": false,
"DisableLogs": false,
"StorageDirectory": "",
"DisableOfflineStorage": false
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
},
"Logging": {
"LogLevel": {
"Default": "Information",
"Microsoft.AspNetCore": "Warning"
}
},
"AllowedHosts": "*"
}