Use Microsoft.Extensions.Diagnostics

[JamesNK]

Fixes #47618

@davidfowl @tarekgh @noahfalk

[JamesNK]

A smooth migration (as expected)

Feedback:

• Should DefaultMeterFactory be public? It would save having to have a TestMeterFactory.
• InstrumentRecorder<T> lets you see measurements that have been collected, but there is no notification that a value has been received. There are a couple of tests where I needed to await new values. Does it make sense to have that notification on the recorder.

[JamesNK]

Cannot reference "Microsoft.Extensions.Diagnostics". This dependency is not in the shared framework. See docs/SharedFramework.md for instructions on how to modify what is in the shared framework.

Microsoft.Extensions.Diagnostics needs to be in the shared framework so it can be referenced by ASP.NET Core bits in the shared framework (Hosting, Kestrel, etc)

[JamesNK]

@wtgodbe This PR adds references to Microsoft.Extensions.Diagnostics. Could you double-check the PR and make sure I have performed the necessary rituals?

[noahfalk]

A smooth migration (as expected)

Excellent 👍

Should DefaultMeterFactory be public? It would save having to have a TestMeterFactory

serviceProvider.GetRequiredService<IMeterFactory>() is insufficient?

There are a couple of tests where I needed to await new values. Does it make sense to have that notification on the recorder.

Tarek originally had one but I believed it would have no use and suggested we get rid of it. So egg on my face :) Do you have a good example so I can better understand where you found it useful?

[JamesNK]

serviceProvider.GetRequiredService() is insufficient?

That works if you have a DI container setup in your test. Without a DI container, you need to create your own implementation.

Tarek originally had one but I believed it would have no use and suggested we get rid of it. So egg on my face :) Do you have a good example so I can better understand where you found it useful?

He might have seen it in the ASP.NET Core prototype.

• InstrumentRecorder with a hacky callback API: https://github.com/dotnet/aspnetcore/blob/4e17e96258aebedd51d4dedb8fab201f7f3d600c/src/Shared/Metrics/InstrumentRecorder.cs
• Example of a test that uses it:

  aspnetcore/src/Middleware/Diagnostics/test/UnitTests/DeveloperExceptionPageMiddlewareTest.cs

  Lines 536 to 589 in 4e17e96

  	[Fact]
  	public async Task UnhandledError_ExceptionNameTagAdded()
  	{
  	// Arrange
  	var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
  	var meterFactory = new TestMeterFactory();
  	var meterRegistry = new TestMeterRegistry(meterFactory.Meters);
  	var instrumentRecorder = new InstrumentRecorder<double>(meterRegistry, "Microsoft.AspNetCore.Hosting", "request-duration");
  	instrumentRecorder.Register(m =>
  	{
  	tcs.SetResult();
  	});
  	using var host = new HostBuilder()
  	.ConfigureServices(s =>
  	{
  	s.AddSingleton<IMeterFactory>(meterFactory);
  	s.AddSingleton(LoggerFactory);
  	})
  	.ConfigureWebHost(webHostBuilder =>
  	{
  	webHostBuilder
  	.UseTestServer()
  	.Configure(app =>
  	{
  	app.UseDeveloperExceptionPage();
  	app.Run(context =>
  	{
  	throw new Exception("Test exception");
  	});
  	});
  	}).Build();
  	await host.StartAsync();
  	var server = host.GetTestServer();
  	// Act
  	var response = await server.CreateClient().GetAsync("/path");
  	Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);
  	await tcs.Task.DefaultTimeout();
  	// Assert
  	Assert.Collection(
  	instrumentRecorder.GetMeasurements(),
  	m =>
  	{
  	Assert.True(m.Value > 0);
  	Assert.Equal(500, (int)m.Tags.ToArray().Single(t => t.Key == "status-code").Value);
  	Assert.Equal("System.Exception", (string)m.Tags.ToArray().Single(t => t.Key == "exception-name").Value);
  	});
  	}

I've worked around these two issues (an IMeterFactory implementation for tests and a way to get notifications of new measurements here)

[tarekgh]

Don't you need the Microsoft.Extensions.Diagnostics.Abstraction too? or will the dependency be automatically picked?

[tarekgh]

CC @joperezr

[tarekgh]

That works if you have a DI container setup in your test. Without a DI container, you need to create your own implementation.

You can see what we are doing in our tests. create a temporary ServiceCollection, call AddMetrics then request IMeterFactory from the service. It doesn't matter if you are running with a container or not.

[noahfalk]

Example of a test that uses it:

        var response = await server.CreateClient().GetAsync("/path");
        Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);
        ...

In this example isn't awaiting the GetAsync() call sufficient to know that the work has been done and the measurement is recorded? Is there some asynchronous work happening somewhere even after the await completes?

[JamesNK]

In this example isn't awaiting the GetAsync() call sufficient to know that the work has been done and the measurement is recorded?

In this particular test, there is a race condition. The server can return the failing response to the client before the metric is recorded.

99% of the time it is fine, but 1% of the time the client can report it's finished before the request is finished on the server and the counter is recorded.

[eerhardt]

This change was merged into #48407. Closing.

[JamesNK]

In this example isn't awaiting the GetAsync() call sufficient to know that the work has been done and the measurement is recorded?

In this particular test, there is a race condition. The server can return the failing response to the client before the metric is recorded.

99% of the time it is fine, but 1% of the time the client can report it's finished before the request is finished on the server and the counter is recorded.

Created issue: dotnet/runtime#86783

[JamesNK]

That works if you have a DI container setup in your test. Without a DI container, you need to create your own implementation.

You can see what we are doing in our tests. create a temporary ServiceCollection, call AddMetrics then request IMeterFactory from the service. It doesn't matter if you are running with a container or not.

I think this is a little gross. However, it's not a big deal to create a simple IMeterFactory if you don't want to involve a container at all. Can leave it as is for now.

[noahfalk]

In this particular test, there is a race condition. The server can return the failing response to the client before the metric is recorded.

AH interesting. In general I wouldn't want to encourage users to write tests with timed waits for a callback if they had an option to avoid it, but I'm fine for us to add a callback API and use docs/comments to provide that guidance.

[ghost]

Hi @noahfalk. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.