HealthCheckPublisherError and HealthCheckPublisherTimeout have the same EventId.Id = 104

[KalleOlaviNiemitalo]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

In Microsoft.Extensions.Diagnostics.HealthChecks.HealthCheckPublisherHostedService.EventIds, both HealthCheckPublisherErrorId and HealthCheckPublisherTimeoutId are defined as 104:

aspnetcore/src/HealthChecks/HealthChecks/src/HealthCheckPublisherHostedService.cs

Lines 201 to 202 in 6a6d775

	public const int HealthCheckPublisherErrorId = 104;
	public const int HealthCheckPublisherTimeoutId = 104;

These are then used as LoggerMessageAttribute.EventId, whence they go to EventId.Id.

Expected Behavior

Each event in the same logging category should have a distinct EventId.Id. That would allow the events to be recognized even after they are passed through a logger that uses only EventId.Id and discards EventId.Name, such as Microsoft.Extensions.Logging.EventLog.EventLogLogger.

Steps To Reproduce

No response

Exceptions (if any)

None

.NET Version

6.0.15

Anything else?

This bug has been there ever since the events were added in aspnet/Diagnostics#498.

Event IDs for some Microsoft.AspNetCore.Mvc.Core log messages changed is precedent for making event IDs unique even though it is a breaking change.

[KalleOlaviNiemitalo]

The code generator would warn about the duplicate ID, but the warning has been suppressed.

aspnetcore/src/HealthChecks/HealthChecks/src/HealthCheckPublisherHostedService.cs

Lines 236 to 245 in 6a6d775

	#pragma warning disable SYSLIB1006
	[LoggerMessage(EventIds.HealthCheckPublisherErrorId, LogLevel.Error, "Health check {HealthCheckPublisher} threw an unhandled exception after {ElapsedMilliseconds}ms", EventName = EventIds.HealthCheckPublisherErrorName)]
	private static partial void HealthCheckPublisherError(ILogger logger, IHealthCheckPublisher HealthCheckPublisher, double ElapsedMilliseconds, Exception exception);
	public static void HealthCheckPublisherTimeout(ILogger logger, IHealthCheckPublisher publisher, TimeSpan duration) =>
	HealthCheckPublisherTimeout(logger, publisher, duration.TotalMilliseconds);
	[LoggerMessage(EventIds.HealthCheckPublisherTimeoutId, LogLevel.Error, "Health check {HealthCheckPublisher} was canceled after {ElapsedMilliseconds}ms", EventName = EventIds.HealthCheckPublisherTimeoutName)]
	private static partial void HealthCheckPublisherTimeout(ILogger logger, IHealthCheckPublisher HealthCheckPublisher, double ElapsedMilliseconds);
	#pragma warning restore SYSLIB1006

SYSLIB1006: Multiple logging methods cannot use the same event ID

[mitchdenny]

@eerhardt just wondering if you can cast your mind back on this change (looks like you added the suppressions here). Was there a reason why the suppression was added vs. just making the IDs unique?

[eerhardt]

Unfortunately I don't remember exactly why I suppressed the warning here. My initial thought is because it is a breaking change, and I didn't want to deal with breaking changes in that PR (since it was just refactoring to use the source generator).

But if I didn't think it was correct, I would have logged a follow-up issue for it, but I don't see one logged. So there must have been a reason why we thought it was valid back then, I just don't know what that reason is.

Note also we have duplicate event names elsewhere - see #46072.

[mitchdenny]

Unfortunately I don't remember exactly why I suppressed the warning here. My initial thought is because it is a breaking change, and I didn't want to deal with breaking changes in that PR (since it was just refactoring to use the source generator).

This sounds reasonable. I possibly would have done the same thing ;) @KalleOlaviNiemitalo are you interested in submitting a PR to fix this up?

[eerhardt]

Note the work involved isn't just fixing up the code, it is also documenting the breaking change.

[mitchdenny]

Ah yes, that will probably require the team to march it through the process. Fair enough.

[eerhardt]

Duplicate of #46099.