Skip to content

Conversation

@CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Feb 5, 2024

Follow-up to #5300 (comment)

Changes

  • Updated ILogger path to set LogRecord.Logger to a Logger built from CategoryName.
  • Updated LogRecord.CategoryName getter to return LogRecord.Logger.Name.
  • Updated LogRecord.CategoryName setter to change LogRecord.Logger (if needed).
  • Updated OtlpLogExporter to only look at LogRecord.Logger.Name.

Details

The big benefit to this change is if users start using something which is emitting logs via the Log Bridge API AND they use an exporter coded to expect CategoryName they will now have something set.

One thing @alanwest will like is OtlpLogExporter no longer has any knowledge of CategoryName. You're welcome @alanwest! 😄

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes

@CodeBlanch CodeBlanch added pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package logs Logging signal related labels Feb 5, 2024
@CodeBlanch CodeBlanch requested a review from a team February 5, 2024 20:47
Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

One thing @alanwest will like is OtlpLogExporter no longer has any knowledge of CategoryName.

Yes I do like it. But c'mon don't we all like a little spec compliance?

if (otlpLogRecord != null)
{
var scopeName = logRecord.CategoryName ?? logRecord.Logger?.Name ?? DefaultScopeName;
var scopeName = logRecord.Logger.Name;
Copy link
Member

Choose a reason for hiding this comment

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

In the tests, we are proving that the values of logRecord.Logger.Name and logRecord.CategoryName are the same. Is there any difference in using one over the other in this context?

Copy link
Member

Choose a reason for hiding this comment

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

OTLPExporter can remain agnostic to ILogger specific CategoryName.

Copy link
Member Author

Choose a reason for hiding this comment

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

No practical difference between the two. This is more of a philosophical change which brings OtlpExporter closer to the spec. The goal being it doesn't know anything about any specific logging framework (ILogger being one of those).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logs Logging signal related pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants