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

Update Components to use LoggerMessage #35585

Merged
merged 2 commits into from
Aug 31, 2021
Merged

Conversation

pranavkm
Copy link
Contributor

No description provided.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Aug 21, 2021
@pranavkm pranavkm marked this pull request as ready for review August 22, 2021 03:20
@pranavkm pranavkm requested a review from a team as a code owner August 22, 2021 03:20
@davidfowl
Copy link
Member

Does blazor have any tests that very logs?

@pranavkm
Copy link
Contributor Author

Does blazor have any tests that very logs?

Probably not? I think there are a few that verify error cases, but we rarely verify the vast majority of log messages.

[LoggerMessage(202, LogLevel.Debug, "Invoking instance method '{MethodIdentifier}' on instance '{DotNetObjectId}' with callback id '{CallId}'.", EventName = "BeginInvokeDotNet")]
private static partial void BeginInvokeDotNet(ILogger logger, string methodIdentifier, long dotNetObjectId, string callId);

[LoggerMessage(217, LogLevel.Debug, "Invoking static method with identifier '{MethodIdentifier}' on assembly '{Assembly}' with callback id '{CallId}'.", EventName = "BeginInvokeDotNetStatic")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change this previously re-used eventId - 202.

[LoggerMessage(203, LogLevel.Debug, "Failed to invoke instance method '{MethodIdentifier}' on instance '{DotNetObjectId}' with callback id '{CallId}'.", EventName = "BeginInvokeDotNetFailed")]
private static partial void BeginInvokeDotNetFailed(ILogger logger, string methodIdentifier, long dotNetObjectId, string callId, Exception exception);

[LoggerMessage(218, LogLevel.Debug, "Failed to invoke static method with identifier '{MethodIdentifier}' on assembly '{Assembly}' with callback id '{CallId}'.", EventName = "BeginInvokeDotNetFailed")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change this previously re-used eventId - 203

[LoggerMessage(204, LogLevel.Debug, "There was an error invoking 'Microsoft.JSInterop.DotNetDispatcher.EndInvoke'.", EventName = "EndInvokeDispatchException")]
public static partial void EndInvokeDispatchException(ILogger logger, Exception ex);

[LoggerMessage(205, LogLevel.Debug, "The JS interop call with callback id '{AsyncCall}' with arguments {Arguments}.", EventName = "EndInvokeJSFailed")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This log statement changed. It used to previously say:

"The JS interop call with callback id '{AsyncCall}' failed with error '{Error}'."

but Error is not value that appears in the code. The calling code does say it should be safe to include the Arguments: in the log message, so this was probably a wrong log statement all along

// We can log the arguments here because it is simply the JS error with the call stack.

Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

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

LGTM; just a couple of nits with regard to log ordering based on ids.

src/Components/Components/src/RenderTree/Renderer.Log.cs Outdated Show resolved Hide resolved
src/Components/Server/src/Circuits/CircuitHost.cs Outdated Show resolved Hide resolved
src/Components/Server/src/Circuits/CircuitHost.cs Outdated Show resolved Hide resolved
src/Components/Server/src/Circuits/CircuitRegistry.cs Outdated Show resolved Hide resolved
src/Components/Server/src/Circuits/RemoteRenderer.cs Outdated Show resolved Hide resolved
@pranavkm pranavkm force-pushed the prkrishn/components-loggermessage branch from 832ea98 to fc20c11 Compare August 31, 2021 15:45
@pranavkm pranavkm enabled auto-merge (squash) August 31, 2021 15:46
@pranavkm pranavkm merged commit 7f2bb84 into main Aug 31, 2021
@pranavkm pranavkm deleted the prkrishn/components-loggermessage branch August 31, 2021 17:13
@ghost ghost added this to the 7.0-preview1 milestone Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants