-
Notifications
You must be signed in to change notification settings - Fork 751
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
Use the same naming rules for the names of logging dimensions #4545
Conversation
src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/Internal/Log.cs
Outdated
Show resolved
Hide resolved
Use dot "." instead of underscore "_" for tags provider
Is the PR still draft? |
@iliar-turdushev any chance you checked logging categories we use? |
src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingTagNames.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/Internal/Log.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Extensions.Diagnostics.ExtraAbstractions/Logging/LoggerMessageState.TagCollector.cs
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Diagnostics.Extra/Enrichment/ApplicationEnricherTags.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Diagnostics.Extra/Enrichment/ApplicationEnricherTags.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Diagnostics.Extra/Enrichment/ApplicationEnricherTags.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Diagnostics.Extra/Enrichment/ApplicationEnricherTags.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Diagnostics.Extra/Enrichment/ProcessEnricherTagNames.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Diagnostics.Extra/Enrichment/ProcessEnricherTagNames.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Logging/HttpLoggingTagNames.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Logging/HttpLoggingTagNames.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Logging/HttpLoggingTagNames.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Logging/HttpLoggingTagNames.cs
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs
Outdated
Show resolved
Hide resolved
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.
Why do we need to change all the logging templates from {foo} to {Foo}?
@geeknoid |
@iliar-turdushev the PR LGTM, the only missing bit is header names normalization (if we agree on the naming) |
src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingTagNames.cs
Show resolved
Hide resolved
@xakep139 @iliar-turdushev Other than fixing code coverage, is this ready to go in? |
@joperezr We are waiting for review from @reyang and @noahfalk. Also, if we agree to follow OTel's HTTP Request/Response header name semantics, then we'll also need to implement header name normalization. |
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.
This seems fine to me, but for anything nuanced about the conventions I probably wouldn't catch it. If you want a higher level of scrutiny, someone like Liudmila is probably a better reviewer than I.
...Libraries/Microsoft.Extensions.Http.Diagnostics/Microsoft.Extensions.Http.Diagnostics.csproj
Show resolved
Hide resolved
@lmolkova Please, review the PR. Thank you. |
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.
Left a few minor comments on naming.
I'm a bit concerned about the inconsistent naming patterns. otel.attributes and PascalCaseAttributes in the same log record would look messy.
src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingTagNames.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/Internal/Log.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/Internal/Log.cs
Show resolved
Hide resolved
...aries/Microsoft.Extensions.Http.Diagnostics/Logging/Internal/LoggerMessageStateExtensions.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingTagNames.cs
Show resolved
Hide resolved
@lmolkova Thank you very much for reviewing the PR. Yeah, we agree, but, unfortunately, there is no perfect solution here. In our opinion, the best we can do is to follow OTel's semantic-conventions where possible. The rest, which is not yet present in OTel or cannot be aligned with OTel due to implementation details in .NET, will be aligned with .NET's naming rules. This, at least, will allow us to leverage some benefits of following OTel's schema, for example, integration with tools built on top of it. cc @klauco |
The following has been implemented:
PascalCase
for the name of the dimension.LogPropertiesAttribute
andTagProviderAttribute
.Microsoft Reviewers: Open in CodeFlow