-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Merge #28698 to branch 5.* #30862
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
Comments
The log is a bit confusing, but is there any more impact to you? An issue like this wouldn't normally qualify for a patch. |
The problem affects our central logging system (EFK stack). We have several apps with diffrent .Net Core version 3., 5. etc. In version 3.* there was no bug: https://github.com/dotnet/aspnetcore/blob/v3.1.13/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs#L50 Bug exists in 5.*: https://github.com/dotnet/aspnetcore/blob/v5.0.4/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs#L51 So there is diffrent format for one field when logs are pushed to the elastic. |
This seems like reasonable thing to backport. It's super low risk but I'll let @Pilchie decide. |
Triage: we're in favor of patching. This is a regression from 3.1 to 5.0. We could technically provide a private reflection workaround, but this should be patch worthy. |
Fixed by #31080 |
Description
In 5.0 some logging updates were made that accidentally swapped the argument parameters for a log.
Customer Impact
Customers consuming structured logging might depend on the types of the structured data, swapping the arguments can break them as seen above, also the log reads incorrectly.
Regression?
Regressed from 3.1 to 5.0 (fixed in 6.0 already)
Risk
The change is a logging change only, just switching the parameter order.
Verification
Packaging changes reviewed?
Hi!
It is possible to merge fix from #28698 to branch 5.*?
Best regards,
Mateusz
The text was updated successfully, but these errors were encountered: