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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 25 additions & 2 deletions examples/AspNetCore/Controllers/WeatherForecastController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

namespace Examples.AspNetCore.Controllers;

using System.Diagnostics;
using System.Diagnostics.Metrics;
using Examples.AspNetCore;
reyang marked this conversation as resolved.
Show resolved Hide resolved
using Microsoft.AspNetCore.Mvc;

[ApiController]
Expand All @@ -24,16 +27,22 @@ 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<long> freezingDaysCounter;

public WeatherForecastController(ILogger<WeatherForecastController> logger)
public WeatherForecastController(ILogger<WeatherForecastController> logger, Instrumentation instrumentation)
{
this.logger = logger ?? throw new ArgumentNullException(nameof(logger));

ArgumentNullException.ThrowIfNull(instrumentation);
this.activitySource = instrumentation.ActivitySource;
Copy link

Choose a reason for hiding this comment

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

Nit: How about we store the Instrumentation object directly in a single field instead of one field per source and counter. It doesn't make a big difference for this sample, but for folks who have many counters it avoids adding lots of fields to their type if they apply the same pattern.

Copy link
Member

Choose a reason for hiding this comment

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

I suggested this pattern to simply code elsewhere, but I can see merits to your suggestion too.

this.instrumentation.activitySource.StartActivity, this.instrumentation.counter.Add
vs
this.activitySource.StartActivity, this.counter.Add

Considering this as non-blocking, so proceeding to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you asking to revert this suggestion? I will let you and @cijothomas decide.

this.freezingDaysCounter = instrumentation.FreezingDaysCounter;
}

[HttpGet]
Expand All @@ -45,6 +54,17 @@ 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;

// Optional: Manually create an activity. This will become a child of
// the activity created from the instrumentation library for AspNetCore.
// Manually created activities are useful when there is a desire to track
// a specific subset of the request. In this example one could imagine
// that calculating the forecast is an expensive operation and therefore
// something to be distinguished from the overall request.
// Note: Tags can be added to the current activity without the need for
// a manual activity using Acitivty.Current?.SetTag()
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 +74,9 @@ public IEnumerable<WeatherForecast> Get()
})
.ToArray();

// Optional: Count the freezing days
this.freezingDaysCounter.Add(forecast.Count(f => f.TemperatureC < 0));
Copy link

Choose a reason for hiding this comment

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

People might be confused if they think about the meaning of this counter. Presumably if a web service is issuing weather reports it is probably giving reports for the same days over and over which means this counter will have a value something like num_requested_reports * num_freezing_days_per_report. How about a counter that tracks the number of forecasts instead? That value both seems more plausibly useful and makes more sense to use a Counter to track it.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea. We can add forecast as a dimension.
Lets address in a follow up.

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 was trying to find a "real" enough use case but didn't want to change the logic of the example. Would you define a forecast here as single invocation of this endpoint? In that case this information is the same as number of requests which one can get from the histogram created by AddAspNetCoreInstrumentation.

Copy link

Choose a reason for hiding this comment

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

Yeah, each invocation of this method would be Add(1) in my suggestion. I agree it is duplicative most of the time (they might not have those other metrics enabled) but that still seemed like an improvement. If we wanted something that is both reasonable and unique we'd probably need to change the sample at least a little. For example the forecast service could have a cache and the counter counts how many requests weren't found in the cache. Another option could be forecast requests are for a variable number of days and the counter counts how many 10 day forecasts were requested (maybe 10 day forecasts are more expensive to produce than shorter forecasts?).

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 tried capturing this in a new issue (https://github.com/open-telemetry/opentelemetry-dotnet/issues/4166) so this does not get lost. If you feel extra clarity is needed please add it to the issue.


this.logger.LogInformation(
"WeatherForecasts generated {count}: {forecasts}",
forecast.Length,
Expand Down
50 changes: 50 additions & 0 deletions examples/AspNetCore/Instrumentation.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// <copyright file="Instrumentation.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

namespace Examples.AspNetCore;

using System.Diagnostics;
using System.Diagnostics.Metrics;

/// <summary>
/// It is recommended to use a custom type to hold references for
/// ActivitySource and Instruments. This avoids possible type collisions
/// with other components in the DI container.
/// </summary>
public class Instrumentation : IDisposable
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
reyang marked this conversation as resolved.
Show resolved Hide resolved
{
internal const string ActivitySourceName = "Examples.AspNetCore";
internal const string MeterName = "Examples.AspNetCore";
private readonly Meter meter;

public Instrumentation()
{
string? version = typeof(Instrumentation).Assembly.GetName().Version?.ToString();
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
this.ActivitySource = new ActivitySource(ActivitySourceName, version);
this.meter = new Meter(MeterName, version);
this.FreezingDaysCounter = this.meter.CreateCounter<long>("weather.days.freezing", "The number of days where the temperature is below freezing");
}

public ActivitySource ActivitySource { get; }

public Counter<long> FreezingDaysCounter { get; }

public void Dispose()
{
this.ActivitySource.Dispose();
this.meter.Dispose();
}
}
12 changes: 10 additions & 2 deletions examples/AspNetCore/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// limitations under the License.
// </copyright>

using System.Reflection;
using Examples.AspNetCore;
Copy link
Member

Choose a reason for hiding this comment

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

"using" or "namespace"?

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 think using is correct. This allows me to reference Instrumentation here. As far as I know you cannot define a namespace with these top level statements

using OpenTelemetry;
using OpenTelemetry.Exporter;
using OpenTelemetry.Instrumentation.AspNetCore;
Expand All @@ -37,9 +37,13 @@
// 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: typeof(Program).Assembly.GetName().Version?.ToString() ?? "unknown",
serviceInstanceId: Environment.MachineName);

// Create a service to expose ActivitySource, and Metric Instruments
// for manual instrumentation
appBuilder.Services.AddSingleton<Instrumentation>();

// Configure OpenTelemetry tracing & metrics with auto-start using the
// StartWithHost extension from OpenTelemetry.Extensions.Hosting.
appBuilder.Services.AddOpenTelemetry()
Expand All @@ -48,7 +52,9 @@
{
// Tracing

// Ensure the TracerProvider subscribes to any custom ActivitySources.
builder
.AddSource(Instrumentation.ActivitySourceName)
.SetSampler(new AlwaysOnSampler())
.AddHttpClientInstrumentation()
.AddAspNetCoreInstrumentation();
Expand Down Expand Up @@ -98,7 +104,9 @@
{
// Metrics

// Ensure the MeterProvider subscribes to any custom Meters.
builder
.AddMeter(Instrumentation.MeterName)
.AddRuntimeInstrumentation()
.AddHttpClientInstrumentation()
.AddAspNetCoreInstrumentation();
Expand Down