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

[release/6.0] [Blazor] Fix duplicated place holder in log message #53009

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

gayaK
Copy link

@gayaK gayaK commented Dec 26, 2023

Fix duplicated place holder in log message

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Fix bug of log message in .NET6 or earlyer.

Description

Microsoft.AspNetCore.Components.RenderTree.Renderer+Log.InitializingChildComponent() has duplicated place holder ParentComponentId.

However, .NET7 or later are already fixed by #35585

Customer Impact

This PR can avoid frequent exceptions on some logging frameworks.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

The logic has not been modified, only the message has been modified.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

`ParentComponentId` is duplicated in `Microsoft.AspNetCore.Components.RenderTree.Renderer+Log.InitializingChildComponent`.
@gayaK gayaK requested a review from a team as a code owner December 26, 2023 00:06
@ghost ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Dec 26, 2023
@ghost
Copy link

ghost commented Dec 26, 2023

Thanks for your PR, @gayaK. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@ghost ghost added this to the 6.0.x milestone Dec 26, 2023
@ghost
Copy link

ghost commented Dec 26, 2023

Hi @gayaK. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@gayaK gayaK changed the title Fix duplicated place holder in log message [release/6.0] [Blazor] Fix duplicated place holder in log message Dec 26, 2023
Copy link
Member

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

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

Thanks @gayaK.
Is this actually something that impacts you and if so can you share more details about your scenario?
Servicing pretty much anything has an inherent risk to it that we'd rather avoid.

@mkArtakMSFT mkArtakMSFT self-assigned this Dec 27, 2023
@mkArtakMSFT mkArtakMSFT added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Dec 29, 2023
@gayaK
Copy link
Author

gayaK commented Jan 5, 2024

Thank you for reviewing and approving my PR.

This update resolves a bug that leads to decreased performance under specific circumstances:

  • When utilizing Blazor on .NET 6
  • When using specific logging frameworks (such as NLog)
  • When debug-level logging is enabled
  • During the initialization of nested Blazor components every time

I believe the benefits outweigh the risks, considering that debug-level logging is seldom enabled in production environments.

Details

The unfixed message in Microsoft.AspNetCore.Components.RenderTree.Renderer will be transformed into FormattedLogValues, containing the following key-value pairs:

Key Value Type Correct Key
ComponentId int ComponentId
ComponentType type ComponentType
ParentComponentId int ParentComponentId
ParentComponentId type ParentComponentType

Nevertheless, some of these frameworks convert the list into a dictionary, leading to exceptions caused by duplicated keys.

The following is a part of the stack trace that causes the exception in NLog.

  1. NLog.Extensions.Logging.NLogLogger.CreateLogEventInfo‎
  2. NLog.LogEventInfo.ctor
  3. NLog.Internal.PropertiesDictionary.InsertMessagePropertiesIntoEmptyDictionary

After catching that exception, key-value pairs are converted to the following:

Key Value Type
ComponentId int
ComponentType type
ParentComponentId int
ParentComponentId_1 type

I also intend to report this issue to NLog.

@mkArtakMSFT mkArtakMSFT added Servicing-consider Shiproom approval is required for the issue and removed pr: pending author input For automation. Specifically separate from Needs: Author Feedback labels Jan 6, 2024
@ghost
Copy link

ghost commented Jan 6, 2024

Hi @gayaK. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jan 8, 2024
@ghost
Copy link

ghost commented Jan 8, 2024

Hi @gayaK. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@mkArtakMSFT mkArtakMSFT merged commit 0df5b8a into dotnet:release/6.0 Jan 8, 2024
23 checks passed
@ghost ghost modified the milestones: 6.0.x, 6.0.27 Jan 8, 2024
@mkArtakMSFT
Copy link
Member

Thank you for your contribution, @gayaK!

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 community-contribution Indicates that the PR has been added by a community member Servicing-approved Shiproom has approved the issue
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants