-
Notifications
You must be signed in to change notification settings - Fork 405
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 target as LogRecord field, and using it as scope.name in OTLP exporter #1869
Adding target as LogRecord field, and using it as scope.name in OTLP exporter #1869
Conversation
Moving to draft to add tests, but should be good to review otherwise. |
…emetry-rust into set_instrumentation_scope
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1869 +/- ##
=======================================
- Coverage 74.6% 74.6% -0.1%
=======================================
Files 122 122
Lines 19952 20001 +49
=======================================
+ Hits 14902 14933 +31
- Misses 5050 5068 +18 ☔ View full report in Codecov by Sentry. |
…emetry-rust into set_instrumentation_scope
opentelemetry-otlp/CHANGELOG.md
Outdated
@@ -18,6 +18,7 @@ now use `.with_resource(RESOURCE::default())` to configure Resource when using | |||
- Bump MSRV to 1.70 [#1840](https://github.com/open-telemetry/opentelemetry-rust/pull/1840) | |||
- Fixing the OTLP HTTP/JSON exporter. [#1882](https://github.com/open-telemetry/opentelemetry-rust/pull/1882) - The exporter was broken in the | |||
previous release. | |||
- **Breaking** [1869](https://github.com/open-telemetry/opentelemetry-rust/pull/1869) The OTLP logs exporter now populates the [InstrumentationScope::name](https://github.com/open-telemetry/opentelemetry-proto/blob/b3060d2104df364136d75a35779e6bd48bac449a/opentelemetry/proto/common/v1/common.proto#L73) field with the name of the target/component emitting the logs, instead of the appender's name as done previously. |
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.
OTLP Exporter now overrides InstrumentationScope's name with target
from LogRecord
, if it is present.
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.
Done.
@@ -172,6 +172,7 @@ where | |||
let mut log_record = self.logger.create_log_record(); | |||
log_record.set_severity_number(severity_of_level(meta.level())); | |||
log_record.set_severity_text(meta.level().to_string().into()); | |||
log_record.set_target(meta.target().to_string()); |
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.
the logger created by appender - please modify it to not populate version, as it'll be misleading now.
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.
also - there is some experimental feature where target was added as an attribute. lets remove that now?
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.
the logger created by appender - please modify it to not populate version, as it'll be misleading now.
Other thought - we can keep it, and allow users to customize the logger-name, for those interested to have the single logger, and use that created by appender?
also - there is some experimental feature where target was added as an attribute. lets remove that now?
Yeah good point. This is basically the "log.target" coming from log-to-tracing conversion. Will handle it as separate PR ?
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.
the version will be misleading. it'll be the version of appender-crate, but the name will be that of the tracing:target. I'd suggest to remove version completely to avoid this confusion..
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.
Agree, as of now I have removed version
and attributes
population from OTLP exporter. We should make this configurable in OTLP exporter - default to override the scope.name with target, unless configured otherwise. Will create a separate PR for that (and a tracing task). I think the current way of configuration is bit messy with no separate way to provide log/trace/metrics specific configuration.
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.
#1890 to track this.
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 is basically the "log.target" coming from log-to-tracing conversion. Will handle it as separate PR ?
Lets add an issue to track this too.
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.
#1894 for tracking.
opentelemetry/src/logs/record.rs
Outdated
@@ -10,6 +10,11 @@ pub trait LogRecord { | |||
{ | |||
} | |||
|
|||
/// Sets the `target` of a record |
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.
some note about the need for this as opposed to instrumentation scope would be helpful.
also mention that exporters would use target to override instrumentation scope when present.
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.
Done.
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.
Looks good, overall. Left some comments to be addressed before marking as approved.
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 suggestion to improve changelog wording, rest looks good.
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Fixes #1683
Changes
LogRecord::set_target(&mut self, target: Cow<'static, str>)
.target
as a field inopentelemetry_sdk::logs::LogRecord
:target
toscope.name
before exporting the record.Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes