-
Notifications
You must be signed in to change notification settings - Fork 872
Fix: Search entire attribute list for {OriginalFormat} #6881
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
base: main
Are you sure you want to change the base?
Fix: Search entire attribute list for {OriginalFormat} #6881
Conversation
|
|
9d8d296 to
529f930
Compare
martincostello
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test that would fail without the fix so we know it is indeed fixed?
| @@ -215,14 +215,25 @@ private static bool TryGetOriginalFormatFromAttributes( | |||
| [NotNullWhen(true)] out string? originalFormat) | |||
| { | |||
| if (attributes != null && attributes.Count > 0) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be simplified by just using having the for loop, but going backwards?
for (int i = attributes.Count - 1; i > -1; i--)
{
var attribute = attributes[i];
if (attribute.Key == "{OriginalFormat}" && attribute.Value is string value)
{
originalFormat = value;
return true;
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the backwards loop is cleaner and still checks the most common position (last index) first. Updated!
…mplementation only checked the last position, which failed when MS Telemetry Abstractions generator places it at the first position. Now searches all positions. Fixes open-telemetry#5983
529f930 to
18521a9
Compare
| var lastAttribute = attributes[attributes.Count - 1]; | ||
| if (lastAttribute.Key == "{OriginalFormat}" | ||
| && lastAttribute.Value is string tempOriginalFormat) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| { | |
| { |
The
TryGetOriginalFormatFromAttributesmethod only checked the last position for the{OriginalFormat}attribute. This fails when using Microsoft.Extensions.Telemetry.Abstractions logging source generator, which places the attribute at the first position.Fixes #
This resolves the issue where
LogRecord.Bodycontained the formatted message instead of the template when using the MS Telemetry Abstractions generator.Design discussion issue #
#5983
Changes
{OriginalFormat}appears at any position in the attribute listMerge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes