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

Use the same naming rules for the names of logging dimensions #4545

Merged
merged 21 commits into from
Oct 23, 2023

Conversation

iliar-turdushev
Copy link
Contributor

@iliar-turdushev iliar-turdushev commented Oct 10, 2023

The following has been implemented:

  • Use OTel semantic conventions for the names of logging dimensions where possible. Since semantic conventions for logs are Experimental, and are missed for the most of areas/objects we need to agree on the dimensions names, we decided to use semantic conventions for traces. Semantic conventions for traces are also Experimental, but they are defined for a wide variety of areas/objects.
  • If a semantic convention for a dimensions or a log message parameter is missing, then we use PascalCase for the name of the dimension.
  • Use dot "." instead of underscore "_" as a separator for the names of dimension of LogPropertiesAttribute and TagProviderAttribute.
  • Remove "env_" preffix from the names of enrichment dimensions.
Microsoft Reviewers: Open in CodeFlow

@iliar-turdushev
Copy link
Contributor Author

@ananthsr If we rename enrichment property latencyInfo to LatencyInfo (the first letter is Capital), won't it affect latency measurement? Thank you.

@klauco
Copy link

klauco commented Oct 10, 2023

@ananthsr If we rename enrichment property latencyInfo to LatencyInfo (the first letter is Capital), won't it affect latency measurement? Thank you.

let's ensure we align the naming here regardless of the impact.

@xakep139
Copy link
Contributor

Is the PR still draft?

@xakep139
Copy link
Contributor

@iliar-turdushev any chance you checked logging categories we use?

Copy link
Member

@geeknoid geeknoid left a 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}?

@iliar-turdushev
Copy link
Contributor Author

iliar-turdushev commented Oct 16, 2023

Why do we need to change all the logging templates from {foo} to {Foo}?

@geeknoid
One of the goals of this PR is to be aligned with naming rules/conventions being used in the rest of dotnet, in case there is no a corresponding OTel semantic-convention. In dotnet a PascalCase is being used for the names of logging dimensions and parameters of logger messages. For example: https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/HttpLogging/src/HttpLoggingExtensions.cs#L23C1-L24C93.

@xakep139
Copy link
Contributor

@iliar-turdushev the PR LGTM, the only missing bit is header names normalization (if we agree on the naming)

@joperezr
Copy link
Member

@xakep139 @iliar-turdushev Other than fixing code coverage, is this ready to go in?

@iliar-turdushev
Copy link
Contributor Author

@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.

Copy link
Member

@noahfalk noahfalk left a 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.

@iliar-turdushev
Copy link
Contributor Author

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.

@lmolkova Please, review the PR. Thank you.

Copy link

@lmolkova lmolkova left a 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.

@iliar-turdushev
Copy link
Contributor Author

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.

@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

@RussKie RussKie merged commit ca37d72 into release/8.0 Oct 23, 2023
6 checks passed
@RussKie RussKie deleted the iliarturdu/logging-dim-naming branch October 23, 2023 23:44
@ghost ghost locked as resolved and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
10 participants