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

Make overriding scope.name with logrecord.target configurable in OTLP Log Exporter #2102

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Sep 11, 2024

Fixes open-telemetry/opentelemetry-specification#1890
Design discussion issue (if applicable) #

Changes

  • Added feature flag populate-instrumentation-scope-from-target in Cargo.toml for opentelemetry-otlp and opentelemetry-proto.
  • The flag is enabled by default for both.
  • If flag is enabled scope.name is populated with LogRecord.target. scope.version and other fields are empty.
  • If flag is disabled, scope is populated from opentelemetry-sdk's "InstrumentatonLibrary".

output for:

    info!(name: "my-event", target: "my-target", "hello from {}. My price is {}", "apple", 1.99);

feature flag disabled


2024-09-11T09:33:21.399Z        info    LogsExporter    {"kind": "exporter", "data_type": "logs", "name": "debug", "resource logs": 1, "log records": 1}
2024-09-11T09:33:21.399Z        info    ResourceLog #0
Resource SchemaURL:
Resource attributes:
     -> service.name: Str(basic-otlp-example)
ScopeLogs #0
ScopeLogs SchemaURL:
InstrumentationScope opentelemetry-appender-tracing 0.25.0
LogRecord #0
ObservedTimestamp: 2024-09-11 09:33:21.3943082 +0000 UTC
Timestamp: 1970-01-01 00:00:00 +0000 UTC
SeverityText: INFO
SeverityNumber: Info(9)
Body: Str(hello from apple. My price is 1.99)
Trace ID:
Span ID:
Flags: 0
        {"kind": "exporter", "data_type": "logs", "name": "debug"}

feature flag enabled


2024-09-11T09:33:51.736Z        info    LogsExporter    {"kind": "exporter", "data_type": "logs", "name": "debug", "resource logs": 1, "log records": 1}
2024-09-11T09:33:51.737Z        info    ResourceLog #0
Resource SchemaURL:
Resource attributes:
     -> service.name: Str(basic-otlp-example)
ScopeLogs #0
ScopeLogs SchemaURL:
InstrumentationScope my-target
LogRecord #0
ObservedTimestamp: 2024-09-11 09:33:51.7344587 +0000 UTC
Timestamp: 1970-01-01 00:00:00 +0000 UTC
SeverityText: INFO
SeverityNumber: Info(9)
Body: Str(hello from apple. My price is 1.99)
Trace ID:
Span ID:
Flags: 0
        {"kind": "exporter", "data_type": "logs", "name": "debug"}

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 77.9%. Comparing base (e206533) to head (48463eb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-proto/src/transform/logs.rs 58.3% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   open-telemetry/opentelemetry-specification#2102     +/-   ##
=======================================
- Coverage   78.0%   77.9%   -0.1%     
=======================================
  Files        121     121             
  Lines      20939   20944      +5     
=======================================
+ Hits       16335   16336      +1     
- Misses      4604    4608      +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lalitb lalitb marked this pull request as ready for review September 11, 2024 09:51
@lalitb lalitb requested a review from a team September 11, 2024 09:51
@lalitb lalitb changed the title Make overriding scope.name with logrecord.target configurable in OTLP Log Exporter Make overriding scope.name with logrecord.target configurable in OTLP Log Exporter Sep 11, 2024
@@ -3,6 +3,10 @@
## vNext

- Fix JSON serialization of `metrics::Exemplar` and `trace::span::Link` [#2069](https://github.com/open-telemetry/opentelemetry-rust/pull/2069)
- [2102](https://github.com/open-telemetry/opentelemetry-rust/pull/2102)
Added feature flag `populate-instrumentation-scope-from-target`, enabled by default.
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/open-telemetry/opentelemetry-specification/issues/4154
I am wondering if we should offer this feature flag at all, given the target should be populated as scope always. The appender-name and version can be attributes on the scope. Need to follow the above spec issue before closely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. We can instead have a feature flag to populate the appender-name/version as attributes, and keep target as scope as we do currently.

Copy link
Member

Choose a reason for hiding this comment

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

I think that could work, we need to wait for the spec issue to get resolved. For now, we can continue to keep target as scope. what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we can close this PR and make a decision once the spec issue is resolved

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.

2 participants