Convert HealthChecks logging to use new Logging Source Generator

[eerhardt]

PR Title
Convert HealthChecks logging to use new Logging Source Generator

PR Description
Simplying the HealthChecks logging code by using the new Logging source generator feature.

See dotnet/runtime#36022

cc @maryamariyan

[pranavkm]

[davidfowl]

I would prefer to keep the nested Log class

[eerhardt]

I would prefer to keep the nested Log class

And always pass in _logger?

[davidfowl]

Yep. I don't like the log methods in the same class as where the logging happens

[eerhardt]

Ok, sounds good. I'll move them back.

Just to be sure, do we think the feature of finding the ILogger _logger instance field is still valuable to certain devs? (Just not ASP.NET library devs?) Or should we remove it and encourage people to use a static nested class?

[eerhardt]

Oh, I remember why I did this now. The source generator doesn't support nested classes.

Severity	Code	Description	Project	File	Line	Suppression State
Error	SYSLIB1004	Logging class cannot be in nested types	Microsoft.Extensions.Diagnostics.HealthChecks	C:\git\aspnetcore\src\HealthChecks\HealthChecks\src\DefaultHealthCheckService.cs	201	

So I would need to move the Log class outside of the Service classes.

[davidfowl]

So I would need to move the Log class outside of the Service classes.

OR fix issue? Is it possible to mark both as partial and make the current style work?

[eerhardt]

OR fix issue? Is it possible to mark both as partial and make the current style work?

It isn't possible today because this is an explicit limitation in the source generator. I opened dotnet/runtime#52301 to address it.

[eerhardt]

@davidfowl - do you want me to wait for the source generator support for nested classes? Or should I address the other feedback in this PR to move it forward?

[davidfowl]

I would do the places that don't need the nested class style change first (I think we have a few of them).

[eerhardt]

I was just going to do 1 place as part of "app building" for preview4. This was the first one that jumped out to me 😄 .

This PR can wait until the support comes in the source generator.

[eerhardt]

Thanks for all the feedback. I believe I have addressed it and this is now ready for review. PTAL.

[pranavkm]

Undo?

[eerhardt]

Even though the underlying roslyn issue is closed, I'm still seeing the warnings in VS without this change

Note I am using VS Version 16.11.0 Preview 2.0

[eerhardt]

The reason this is still needed is because we are using an older Roslyn here:

aspnetcore/eng/Versions.props

Lines 161 to 162 in b9efadc

	<!-- Packages from dotnet/roslyn -->
	<MicrosoftNetCompilersToolsetVersion>3.10.0-1.final</MicrosoftNetCompilersToolsetVersion>

That version didn't have the fix for dotnet/roslyn#52527

[pranavkm]

@Tratcher / @JamesNK / @BrennanConroy could you have a look at this PR and agree on the pattern for specifying event ids? It's the only part of that isn't mechanical.

[JamesNK]

I think specifying them in the attribute is fine. Adding a static class with constants doesn't add anything.

[BrennanConroy]

I think having the IDs in attributes is fine, but only if there isn't any other code in-between all the attributes like there is currently. As soon as you start putting random gaps in the grouping you make it a lot easier to introduce duplicate IDs.

Clean:

aspnetcore/src/HealthChecks/HealthChecks/src/DefaultHealthCheckService.cs

Lines 211 to 215 in 0d037c0

	[LoggerMessage(EventIds.HealthCheckProcessingEndId, LogLevel.Debug, "Health check processing with combined status {HealthStatus} completed after {ElapsedMilliseconds}ms", EventName = EventIds.HealthCheckProcessingEndName)]
	private static partial void HealthCheckProcessingEnd(ILogger logger, HealthStatus HealthStatus, double ElapsedMilliseconds);
	[LoggerMessage(EventIds.HealthCheckBeginId, LogLevel.Debug, "Running health check {HealthCheckName}", EventName = EventIds.HealthCheckBeginName)]
	public static partial void HealthCheckBegin(ILogger logger, string HealthCheckName);

Not clean:

aspnetcore/src/HealthChecks/HealthChecks/src/DefaultHealthCheckService.cs

Lines 227 to 253 in 0d037c0

	[LoggerMessage(EventIds.HealthCheckEndId, LogLevel.Error, HealthCheckEndText, EventName = EventIds.HealthCheckEndName)]
	private static partial void HealthCheckEndUnhealthy(ILogger logger, string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription, Exception? exception);
	#pragma warning restore SYSLIB1006
	public static void HealthCheckEnd(ILogger logger, HealthCheckRegistration registration, HealthReportEntry entry, TimeSpan duration)
	{
	switch (entry.Status)
	{
	case HealthStatus.Healthy:
	HealthCheckEndHealthy(logger, registration.Name, entry.Status, duration.TotalMilliseconds, entry.Description);
	break;
	case HealthStatus.Degraded:
	HealthCheckEndDegraded(logger, registration.Name, entry.Status, duration.TotalMilliseconds, entry.Description);
	break;
	case HealthStatus.Unhealthy:
	HealthCheckEndUnhealthy(logger, registration.Name, entry.Status, duration.TotalMilliseconds, entry.Description, entry.Exception);
	break;
	}
	}
	[LoggerMessage(EventIds.HealthCheckErrorId, LogLevel.Error, "Health check {HealthCheckName} threw an unhandled exception after {ElapsedMilliseconds}ms", EventName = EventIds.HealthCheckErrorName)]
	private static partial void HealthCheckError(ILogger logger, string HealthCheckName, double ElapsedMilliseconds, Exception exception);
	public static void HealthCheckError(ILogger logger, HealthCheckRegistration registration, Exception exception, TimeSpan duration) =>
	HealthCheckError(logger, registration.Name, duration.TotalMilliseconds, exception);

[Tratcher]

I think we only need the static class for ids when multiple classes share a log category, then you want a central list to avoid duplicates.

[JamesNK]

Doesn't the new logging generator check for duplicates?

[Tratcher]

How does that work when the category is defined separately in the ILogger?

[BrennanConroy]

Looks like they keep track of all IDs in a class
https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs#L254

[JamesNK]

In the case of KestrelTrace, there would still be one uber logging class, but we're passing in different ILogger instances to the various methods. Same design as today. The difference is just replacing the manually defined KestrelTrace with a mostly code-generated one.

[eerhardt]

Is there a recommendation on what to do in this PR? I'm totally fine with making a change to remove the EventIds class.

Personally, I'd lean to keep it as-is because:

1. It is nice to reuse the constants in the unit tests.
2. It leads to a nice place to put the comment about using hard coded strings for the EventName so they don't get changed by code refactoring.
   • As a new person to the codebase, I would have never knew about this rule without someone catching it in a code review.

[pranavkm]

I guess the consensus seems to be to inline the Ids which we could do in other updates if we need to. In particular because this PR has tests that reference these events, I'm fine with them being in a separate type.

If folks feel super strong about this, please chime in. Otherwise let's get this change in.

[danielcweber]

In the latest 6.0-preview docker image, the logger message arguments are mixed up:

This is probably a source generator issue, but I can't figure out whether it has already been fixed.

cc: @maryamariyan

[ghost]

Hi @danielcweber. 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.

[eerhardt]

Note: I debugged into the repro at #34710. I noticed that this PR isn't in 6.0.0-preview6. It was merged after the preview6 snap.

So the issue existed before my change.

[ghost]

Hi @eerhardt. 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.