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

Drop include_trace_context parameter from Logs API/SDK #1133

Merged
merged 9 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions opentelemetry-api/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Removed

- Drop include_trace_context parameter from Logs API/SDK. [#1133](https://github.com/open-telemetry/opentelemetry-rust/issues/1132)
lalitb marked this conversation as resolved.
Show resolved Hide resolved
- Synchronous instruments no longer accepts `Context` while reporting
measurements. [#1076](https://github.com/open-telemetry/opentelemetry-rust/pull/1076).

Expand Down
12 changes: 1 addition & 11 deletions opentelemetry-api/src/global/logs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
attributes: Option<Vec<KeyValue>>,
include_trace_context: bool,
) -> Box<dyn Logger + Send + Sync + 'static>;
}

Expand All @@ -42,15 +41,8 @@
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
attributes: Option<Vec<KeyValue>>,
include_trace_context: bool,
) -> Box<dyn Logger + Send + Sync + 'static> {
Box::new(self.versioned_logger(
name,
version,
schema_url,
attributes,
include_trace_context,
))
Box::new(self.versioned_logger(name, version, schema_url, attributes))

Check warning on line 45 in opentelemetry-api/src/global/logs.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-api/src/global/logs.rs#L45

Added line #L45 was not covered by tests
}
}

Expand Down Expand Up @@ -102,14 +94,12 @@
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
attributes: Option<Vec<KeyValue>>,
include_trace_context: bool,
) -> Self::Logger {
BoxedLogger(self.provider.versioned_logger_boxed(
name.into(),
version,
schema_url,
attributes,
include_trace_context,
))
}
}
Expand Down
17 changes: 3 additions & 14 deletions opentelemetry-api/src/logs/logger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@

/// The interface for emitting [`LogRecord`]s.
pub trait Logger {
/// Emit a [`LogRecord`]. If this `Logger` was created with
/// `include_trace_context` set to `true`, the logger will set the record's
/// [`TraceContext`] to the active trace context, using the current thread's
/// [`Context`].
/// Emit a [`LogRecord`]. If there is active current thread's [`Context`],
/// the logger will set the record's [`TraceContext`] to the active trace context,
///
/// [`Context`]: crate::Context
/// [`TraceContext`]: crate::logs::TraceContext
Expand All @@ -24,21 +22,12 @@
/// The `name` should be the application name or the name of the library
/// providing instrumentation. If the name is empty, then an
/// implementation-defined default name may be used instead.
///
/// If `include_trace_context` is `true`, the newly created [`Logger`]
/// should set the [`TraceContext`] associated with a record to the
/// current thread's active trace context, using [`Context`].
///
/// [`Context`]: crate::Context
/// [`TraceContext`]: crate::logs::TraceContext

fn versioned_logger(
&self,
name: impl Into<Cow<'static, str>>,
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
attributes: Option<Vec<KeyValue>>,
include_trace_context: bool,
) -> Self::Logger;

/// Returns a new logger with the given name.
Expand All @@ -47,6 +36,6 @@
/// providing instrumentation. If the name is empty, then an
/// implementation-defined default name may be used instead.
fn logger(&self, name: impl Into<Cow<'static, str>>) -> Self::Logger {
self.versioned_logger(name, None, None, None, true)
self.versioned_logger(name, None, None, None)

Check warning on line 39 in opentelemetry-api/src/logs/logger.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-api/src/logs/logger.rs#L39

Added line #L39 was not covered by tests
}
}
1 change: 0 additions & 1 deletion opentelemetry-api/src/logs/noop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ impl LoggerProvider for NoopLoggerProvider {
_version: Option<Cow<'static, str>>,
_schema_url: Option<Cow<'static, str>>,
_attributes: Option<Vec<KeyValue>>,
_include_trace_context: bool,
) -> Self::Logger {
NoopLogger(())
}
Expand Down
12 changes: 1 addition & 11 deletions opentelemetry-otlp/src/logs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,16 +417,12 @@
/// current crate version, using the configured log exporter.
///
/// [`Logger`]: opentelemetry_sdk::logs::Logger
pub fn simple(
self,
include_trace_context: bool,
) -> Result<opentelemetry_sdk::logs::Logger, LogError> {
pub fn simple(self) -> Result<opentelemetry_sdk::logs::Logger, LogError> {

Check warning on line 420 in opentelemetry-otlp/src/logs.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-otlp/src/logs.rs#L420

Added line #L420 was not covered by tests
Ok(build_simple_with_exporter(
self.exporter_builder
.ok_or(crate::Error::NoExporterBuilder)?
.build_log_exporter()?,
self.log_config,
include_trace_context,
))
}

Expand All @@ -438,23 +434,20 @@
pub fn batch<R: RuntimeChannel<BatchMessage>>(
self,
runtime: R,
include_trace_context: bool,
) -> Result<opentelemetry_sdk::logs::Logger, LogError> {
Ok(build_batch_with_exporter(
self.exporter_builder
.ok_or(crate::Error::NoExporterBuilder)?
.build_log_exporter()?,
self.log_config,
runtime,
include_trace_context,
))
}
}

fn build_simple_with_exporter(
exporter: LogExporter,
log_config: Option<opentelemetry_sdk::logs::Config>,
include_trace_context: bool,
) -> opentelemetry_sdk::logs::Logger {
let mut provider_builder =
opentelemetry_sdk::logs::LoggerProvider::builder().with_simple_exporter(exporter);
Expand All @@ -467,15 +460,13 @@
Some(Cow::Borrowed(env!("CARGO_PKG_VERSION"))),
None,
None,
include_trace_context,
)
}

fn build_batch_with_exporter<R: RuntimeChannel<BatchMessage>>(
exporter: LogExporter,
log_config: Option<opentelemetry_sdk::logs::Config>,
runtime: R,
include_trace_context: bool,
) -> opentelemetry_sdk::logs::Logger {
let mut provider_builder =
opentelemetry_sdk::logs::LoggerProvider::builder().with_batch_exporter(exporter, runtime);
Expand All @@ -488,6 +479,5 @@
Some(Cow::Borrowed("CARGO_PKG_VERSION")),
None,
None,
include_trace_context,
)
}
15 changes: 4 additions & 11 deletions opentelemetry-sdk/src/logs/log_emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
attributes: Option<Vec<opentelemetry_api::KeyValue>>,
include_trace_context: bool,
) -> Logger {
let name = name.into();

Expand All @@ -46,7 +45,6 @@
Logger::new(
InstrumentationLibrary::new(component_name, version, schema_url, attributes),
Arc::downgrade(&self.inner),
include_trace_context,
)
}
}
Expand Down Expand Up @@ -170,7 +168,6 @@
///
/// [`LogRecord`]: opentelemetry_api::logs::LogRecord
pub struct Logger {
include_trace_context: bool,
instrumentation_lib: InstrumentationLibrary,
provider: Weak<LoggerProviderInner>,
}
Expand All @@ -179,10 +176,8 @@
pub(crate) fn new(
instrumentation_lib: InstrumentationLibrary,
provider: Weak<LoggerProviderInner>,
include_trace_context: bool,
) -> Self {
Logger {
include_trace_context,
instrumentation_lib,
provider,
}
Expand Down Expand Up @@ -210,12 +205,10 @@
let config = provider.config();
for processor in provider.log_processors() {
let mut record = record.clone();
if self.include_trace_context {
let ctx = Context::current();
if ctx.has_active_span() {
let span = ctx.span();
record.trace_context = Some(span.span_context().into());
}
let ctx = Context::current();
if ctx.has_active_span() {
let span = ctx.span();
record.trace_context = Some(span.span_context().into());

Check warning on line 211 in opentelemetry-sdk/src/logs/log_emitter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/logs/log_emitter.rs#L208-L211

Added lines #L208 - L211 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we merged #1140. Maybe use map_current here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have resolved the merge conflicts now, and using map_current for active context.

}
let data = LogData {
record,
Expand Down
Loading