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

[examples] Add manual activities and custom metrics to ASP.NET Core example #4133

Merged
merged 15 commits into from
Feb 7, 2023
Merged

[examples] Add manual activities and custom metrics to ASP.NET Core example #4133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
232ab2b
[examples] Add manual activities and custom metrics to ASP.NET Core e…
danelson Jan 31, 2023
62779b2
Use statics for custom ActivitySource and Meter
danelson Feb 1, 2023
8da9a2d
Remove unused using statements
danelson Feb 1, 2023
6d791de
Expose ActivitySource, Meters, and Instruments via a TelemetryService
danelson Feb 2, 2023
8973699
More efficient assembly access
danelson Feb 2, 2023
3961147
Remove interface, rename instrumentation container and dispose
danelson Feb 3, 2023
5d71484
Make Meter private
danelson Feb 7, 2023
660c721
Use long instead of int for counter
danelson Feb 7, 2023
c013789
Clarify comments
danelson Feb 7, 2023
4f7668d
Clarify names for ActivitySource and Meter
danelson Feb 7, 2023
813df21
Rework ActivitySource and Instrument usage
danelson Feb 7, 2023
ce8c48c
Clarify comments for manual instrumentation
danelson Feb 7, 2023
dc516aa
Clarify purpose of Instrumentation class
danelson Feb 7, 2023
d241178
Merge branch 'main' into manual-instrumentation-aspnetcore-example
cijothomas Feb 7, 2023
4352fc1
Fix line endings
danelson Feb 7, 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
21 changes: 19 additions & 2 deletions examples/AspNetCore/Controllers/WeatherForecastController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

namespace Examples.AspNetCore.Controllers;

using System.Diagnostics;
using System.Diagnostics.Metrics;
using Microsoft.AspNetCore.Mvc;

[ApiController]
Expand All @@ -24,16 +26,23 @@ public class WeatherForecastController : ControllerBase
{
private static readonly string[] Summaries = new[]
{
"Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching",
"Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching",
};

private static readonly HttpClient HttpClient = new();

private readonly ILogger<WeatherForecastController> logger;
private readonly ActivitySource activitySource;
private readonly Counter<int> counter;

public WeatherForecastController(ILogger<WeatherForecastController> logger)
public WeatherForecastController(ILogger<WeatherForecastController> logger, ActivitySource activitySource, Meter meter)
Copy link
Member

Choose a reason for hiding this comment

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

@dnelson-relativity In the current shape of the .NET API I think it is an anti-pattern to use DI for ActivitySource & Meter. They are really intended to be used statically. If you really want to inject them, might be better to create a telemetry helper class which exposes source + meter for the app logic and can be safely injected.

/cc @noahfalk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change this to static but I am curious if you could provide more context as to the reason why. I think the DI pattern is a bit more natural for an AspNetCore developer so that is what I chose. Perhaps the examples/docs should speak to that reasoning?

In this case is there something fundamentally different between using it as a singleton and using it statically?

This example is pretty simple so I can just new up the ActivitySource/Meter in WeatherForecastController but I wouldn't want to make people think they should be doing that in every controller. Do you think that is ok or should I create these elsewhere?

Copy link
Member

Choose a reason for hiding this comment

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

In this case is there something fundamentally different between using it as a singleton and using it statically?

The main issue conceptually is it is perfectly acceptable and expected there will many ActivitySources and many Meters in a process so who wins when multiple things call AddSingleton? 😄 Here is what ASP.NET Core does so this code registering its own singleton is probably taking over ASP.NET Core's and causing some very odd telemetry!

This example is pretty simple so I can just new up the ActivitySource/Meter in WeatherForecastController but I wouldn't want to make people think they should be doing that in every controller. Do you think that is ok or should I create these elsewhere?

Probably best to keep it simple and just new them up in the controller. Best practice would probably be build something which plugs into MVC like an action filter (are those still a thing?) but that might be more code than it is worth for an example. It is very hard to show something useful without too much code in these docs/examples! If you are super passionate about it, we could have a more advanced example. Not sure how everyone would feel about that, but I'm not opposed to it.

Copy link
Contributor Author

@danelson danelson Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see what you mean. If I update the TracerProvider configuration to the following and disable the auto instrumentation

builder
    .AddSource("Microsoft.AspNetCore", "Examples.AspNetCore")
    .SetSampler(new AlwaysOnSampler());
    //.AddAspNetCoreInstrumentation() -> Disabled

I get this unexpected result
image
Interestingly this doesn't happen with only the auto instrumentation which is why I never encountered this before.

I updated the example to use statics. On the fence about the "Examples.AspNetCore" magic string but it removes the need for some global constant or passing of additional configuration to the controller. Part of me thinks it is more explicit with the duplication.

Copy link

Choose a reason for hiding this comment

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

I don't know that it is written down anywhere, but we had past discussions with @davidfowl on this topic and agreed that:

  • It was a mistake for ASP.NET to directly inject an instance of ActivitySource, but given back-compat concerns for now we are keeping the mistake as-is rather than changing it.
  • In general developers shouldn't add types to a DI container if their code doesn't define the type. If you want to access an instance of some common type, a better pattern is to define a type that will contain it and inject the containing type. For example instead of injecting ActivitySource, define class MyLibraryTelemetry { ActivitySource Source; } and inject MyLibraryTelemetry instead. This avoids the type collisions with other components that might also like to locate an ActivitySource via the DI container.

{
this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
this.activitySource = activitySource ?? throw new ArgumentNullException(nameof(activitySource));
ArgumentNullException.ThrowIfNull(meter);

// Create a custom metric
this.counter = meter.CreateCounter<int>("weather.days.freezing", "The number of days where the temperature is below freezing");
}

[HttpGet]
Expand All @@ -45,6 +54,11 @@ public IEnumerable<WeatherForecast> Get()
// how dependency calls will be captured and treated
// automatically as child of incoming request.
var res = HttpClient.GetStringAsync("http://google.com").Result;

// Manually create an activity. This will become a child of
// the incoming request.
using var activity = this.activitySource.StartActivity("calculate forecast");

var rng = new Random();
var forecast = Enumerable.Range(1, 5).Select(index => new WeatherForecast
{
Expand All @@ -54,6 +68,9 @@ public IEnumerable<WeatherForecast> Get()
})
.ToArray();

// Count the freezing days
this.counter.Add(forecast.Count(f => f.TemperatureC < 0));

this.logger.LogInformation(
"WeatherForecasts generated {count}: {forecasts}",
forecast.Length,
Expand Down
15 changes: 14 additions & 1 deletion examples/AspNetCore/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// limitations under the License.
// </copyright>

using System.Diagnostics;
using System.Diagnostics.Metrics;
using System.Reflection;
using OpenTelemetry;
using OpenTelemetry.Exporter;
Expand All @@ -34,10 +36,13 @@
// Note: Switch between Console/OTLP by setting UseLogExporter in appsettings.json.
var logExporter = appBuilder.Configuration.GetValue<string>("UseLogExporter").ToLowerInvariant();

var instrumentationScopeName = appBuilder.Configuration.GetValue<string>("InstrumentationScopeName");
var version = Assembly.GetExecutingAssembly().GetName().Version?.ToString() ?? "unknown";

// Build a resource configuration action to set service information.
Action<ResourceBuilder> configureResource = r => r.AddService(
serviceName: appBuilder.Configuration.GetValue<string>("ServiceName"),
serviceVersion: Assembly.GetExecutingAssembly().GetName().Version?.ToString() ?? "unknown",
serviceVersion: version,
serviceInstanceId: Environment.MachineName);

// Configure OpenTelemetry tracing & metrics with auto-start using the
Expand All @@ -48,7 +53,11 @@
{
// Tracing

// Create an ActivitySource for custom instrumentation
// and ensure the TracerPprovider subscribes to it.
appBuilder.Services.AddSingleton(new ActivitySource(instrumentationScopeName, version));
builder
.AddSource(instrumentationScopeName)
.SetSampler(new AlwaysOnSampler())
.AddHttpClientInstrumentation()
.AddAspNetCoreInstrumentation();
Expand Down Expand Up @@ -98,7 +107,11 @@
{
// Metrics

// Create a Meter for custom instrumentation
// and ensure the MeterProvider subscribes to it.
appBuilder.Services.AddSingleton(new Meter(instrumentationScopeName, version));
builder
.AddMeter(instrumentationScopeName)
.AddRuntimeInstrumentation()
.AddHttpClientInstrumentation()
.AddAspNetCoreInstrumentation();
Expand Down
1 change: 1 addition & 0 deletions examples/AspNetCore/appsettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"ParseStateValues": true
}
},
"InstrumentationScopeName": "Examples.AspNetCore",
"ServiceName": "otel-test",
"AllowedHosts": "*",
"UseTracingExporter": "console",
Expand Down