Decide if duplicate EventNames in LoggerMessage is appropriate

[eerhardt]

We have a couple cases caught by the new LoggerMessage analyzer in dotnet/runtime#64667.

Two duplicates here: InvokeDotNetMethodException and InvokeDotNetMethodSuccess

aspnetcore/src/Components/Server/src/Circuits/RemoteJSRuntime.cs

Lines 199 to 209 in 334da01

	[LoggerMessage(2, LogLevel.Debug, "There was an error invoking the static method '[{AssemblyName}]::{MethodIdentifier}' with callback id '{CallbackId}'.", EventName = "InvokeDotNetMethodException")]
	private static partial void InvokeStaticDotNetMethodException(ILogger logger, string assemblyName, string methodIdentifier, string? callbackId, Exception exception);
	[LoggerMessage(4, LogLevel.Debug, "There was an error invoking the instance method '{MethodIdentifier}' on reference '{DotNetObjectReference}' with callback id '{CallbackId}'.", EventName = "InvokeDotNetMethodException")]
	private static partial void InvokeInstanceDotNetMethodException(ILogger logger, string methodIdentifier, long dotNetObjectReference, string? callbackId, Exception exception);
	[LoggerMessage(3, LogLevel.Debug, "Invocation of '[{AssemblyName}]::{MethodIdentifier}' with callback id '{CallbackId}' completed successfully.", EventName = "InvokeDotNetMethodSuccess")]
	private static partial void InvokeStaticDotNetMethodSuccess(ILogger<RemoteJSRuntime> logger, string assemblyName, string methodIdentifier, string? callbackId);
	[LoggerMessage(5, LogLevel.Debug, "Invocation of '{MethodIdentifier}' on reference '{DotNetObjectReference}' with callback id '{CallbackId}' completed successfully.", EventName = "InvokeDotNetMethodSuccess")]
	private static partial void InvokeInstanceDotNetMethodSuccess(ILogger<RemoteJSRuntime> logger, string methodIdentifier, long dotNetObjectReference, string? callbackId);

And a duplicate LocationChangeFailed between:

aspnetcore/src/Components/Server/src/Circuits/CircuitHost.cs

Lines 761 to 762 in 334da01

	[LoggerMessage(210, LogLevel.Debug, "Location change to '{URI}' in circuit '{CircuitId}' failed.", EventName = "LocationChangeFailed")]
	public static partial void LocationChangeFailed(ILogger logger, string uri, CircuitId circuitId, Exception exception);

aspnetcore/src/Components/Server/src/Circuits/CircuitHost.cs

Lines 812 to 813 in 334da01

	[LoggerMessage(219, LogLevel.Error, "Location change to '{URI}' in circuit '{CircuitId}' failed.", EventName = "LocationChangeFailed")]
	public static partial void LocationChangeFailedInCircuit(ILogger logger, string uri, CircuitId circuitId, Exception exception);

I've suppressed these warnings in #46055 for now. But we should make a conscious decision if we really want/need these EventNames to be the same, or if we should change them. My understanding is changing them is technically a breaking change, so we should be sure that they need changing if we do.

[javiercn]

@eerhardt thanks for raising this up.

I believe this is a copy/paste type of error. We should change the event names to be unique

[javiercn]

We probably want the name to be InvokeInstanceDotNetMethodSuccess

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[JamesNK]

@javiercn What is going on with LocationChangeFailed and LocationChangeFailedInCircuit? The log messages are identical apart from the logging level.

The easy fix is changing LocationChangeFailedInCircuit's log name to LocationChangeFailedInCircuit, but what was the idea behind adding it? Why identical logs but different levels?

[JamesNK]

We probably want the name to be InvokeInstanceDotNetMethodSuccess

InvokeInstanceDotNetMethodException and InvokeStaticDotNetMethodException

InvokeInstanceDotNetMethodSuccess and InvokeStaticDotNetMethodSuccess

[JamesNK]

By the way, logging ID and name changes are considered breaking changes and should be added at https://github.com/aspnet/Announcements and friends. e.g. https://github.com/aspnet/Announcements

e.g. aspnet/Announcements#447

[eerhardt]

Note that in #46055, @SteveSandersonMS and I updated the event names that were obviously wrong. We should include them as well in any announcement of event name changes:

• ResponseCaching: ExpirationExpiresExceeded => NotModifiedIfNoneMatchStar
• RedisLog: Connected => NotConnected
• Components CircuitRegistry: FailedToReconnectToCircuit => ReconnectionSucceeded
• Components CircuitHost: BeginInvokeDotNetFailed => BeginInvokeDotNetStaticFailed (to align with BeginInvokeDotNetStatic)

[shiladitya-mukerji-22]

Hey @javiercn, is this issue still open? If yes I would like to work on it.

[javiercn]

@shiladitya-mukerji-22 Yes, if nobody yet has addressed the issue, I think we would accept a change for this.

[shiladitya-mukerji-22]

@javiercn Under the task it shows that no one has been assigned yet. So, could you please assign the task to me.

[javiercn]

@shiladitya-mukerji-22 go for it.

[shiladitya-mukerji-22]

@javiercn Thank you.