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

Adding a property to the LogRecordData struct #5275

Closed
wants to merge 1 commit into from
Closed

Adding a property to the LogRecordData struct #5275

wants to merge 1 commit into from

Conversation

AB027PS
Copy link
Contributor

@AB027PS AB027PS commented Jan 29, 2024

Fixes #5274

Changes

This would add a new property to the LogRecordData struct with a default value, which would allow us to set LogRecord.CategoryName when using the logging bridge API

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
  • Changes in public API reviewed (if applicable)

@AB027PS AB027PS requested a review from a team January 29, 2024 13:24
Copy link

linux-foundation-easycla bot commented Jan 29, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: AB027PS / name: Julius Koval (d1e9171)

@AB027PS AB027PS changed the title commit Adding a property to the LogRecordData struct Jan 29, 2024
Comment on lines +1 to +2
OpenTelemetry.Logs.LogRecordData.CategoryName.get -> string!
OpenTelemetry.Logs.LogRecordData.CategoryName.set -> void
Copy link
Member

@reyang reyang Jan 29, 2024

Choose a reason for hiding this comment

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

Need to understand the user scenario regarding why new APIs need to be introduced. (working around an exporter exception doesn't justify adding new API in the SDK)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, sorry I should have explained this. I created an NLog target in order to export NLog logs using the OtlpExporter. The idea there is that I could create multiple NLog loggers, all logs would be sent to one OpenTelemetry logger and CategoryName of each log would then be set to the name of the original NLog logger.

Copy link
Member

Choose a reason for hiding this comment

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

@AB027PS Right here where you retrieve your Logger: https://github.com/juliuskoval/NLog.Targets.OpenTelemetryProtocol/blob/64022137497508953f227457401d320171e1a7e3/NLog.Targets.OpenTelemetryProtocol/OtlpTarget.cs#L57

At the moment you aren't supplying a name. What you probably want to do is do something like loggerProvider.GetLogger(nlogLoggerInstance.Name) (essentially create a Logger for each NLog logger). That will establish the "instrumentation scope" as the spec describes it. Then if you modify the OtlpExporter to check LogRecord.Logger.Name when it doesn't have a CategoryName everything should work as you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. I created a new PR.

@CodeBlanch
Copy link
Member

I don't think this is a change we're willing to take at this time. LogRecordData and the Log Bridge API is meant to be fully spec-compliant. CategoryName is not something the spec allows: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#emit-a-logrecord

I don't speak for everyone, but I think we would be willing to accept a PR which updates the OtlpExporter to use LogRecord.Logger.Name if LogRecord.CategoryName is NOT set. That would be spec-compliant IMO.

@CodeBlanch CodeBlanch closed this Jan 29, 2024
@cijothomas
Copy link
Member

+1 to what Blanch suggested.

@AB027PS
Copy link
Contributor Author

AB027PS commented Jan 30, 2024

@CodeBlanch I had seen the spec, but the way I understood it is that the bridge API must accept the listed parameters, but that it may accept others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow us to set CategoryName when using the logging bridge API
4 participants