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

appender-tracing: Include trace ID and span ID in logs when nested in tracing crate's spans #2438

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
fb2af02
appender-tracing: Include trace ID and span ID in logs when nested in…
dylanahsmith Dec 13, 2024
d08b480
appender-tracing: Make tracing-opentelemetry integration optional
dylanahsmith Dec 17, 2024
69b467a
Merge branch 'main' into appender-tracing-fix-trace-context
TommyCpp Dec 17, 2024
c6e1141
Make the new feature experimental
dylanahsmith Dec 17, 2024
12cf399
appender-tracing: Remove undesired dependency on opentelemetry_sdk
dylanahsmith Dec 17, 2024
750df51
s/experimental-use-tracing-span-context/experimental_use_tracing_span…
dylanahsmith Dec 18, 2024
319bed7
Merge branch 'main' into appender-tracing-fix-trace-context
lalitb Dec 19, 2024
3146dfb
Merge branch 'main' into appender-tracing-fix-trace-context
lalitb Jan 12, 2025
f7f2e16
Merge branch 'main' into appender-tracing-fix-trace-context
lalitb Jan 21, 2025
80741cd
Remove opentelemetry-prometheus as a workspace member
dylanahsmith Jan 22, 2025
3aa5bdf
Merge branch 'main' into appender-tracing-fix-trace-context
cijothomas Jan 23, 2025
d0b94ce
Merge branch 'main' into appender-tracing-fix-trace-context
lalitb Jan 23, 2025
a53f3fc
Link tracing crate in new changelog entry
dylanahsmith Jan 24, 2025
99580f8
Merge branch 'main' into appender-tracing-fix-trace-context
dylanahsmith Jan 24, 2025
50b549a
Add a comment for the [patch.crates-io] Cargo.toml section
dylanahsmith Jan 24, 2025
260df21
Merge branch 'main' into appender-tracing-fix-trace-context
cijothomas Jan 24, 2025
bd23f55
Merge branch 'main' into appender-tracing-fix-trace-context
cijothomas Jan 25, 2025
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
Next Next commit
appender-tracing: Include trace ID and span ID in logs when nested in…
… spans
  • Loading branch information
dylanahsmith committed Dec 17, 2024
commit fb2af021918fb7adcfd9a01e121a4c7413c90d56
5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,8 @@ tracing = { version = ">=0.1.40", default-features = false }
tracing-core = { version = ">=0.1.33", default-features = false }
tracing-subscriber = { version = "0.3", default-features = false }
url = { version = "2.5", default-features = false }

[patch.crates-io]
opentelemetry = { path = "opentelemetry" }
opentelemetry_sdk = { path = "opentelemetry-sdk" }
opentelemetry-stdout = { path = "opentelemetry-stdout" }
1 change: 1 addition & 0 deletions opentelemetry-appender-tracing/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## vNext

- Bump msrv to 1.75.0.
- Include trace_id & span_id when logging from inside a tracing span


## 0.27.0
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-appender-tracing/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ rust-version = "1.75.0"
[dependencies]
log = { workspace = true, optional = true }
opentelemetry = { version = "0.27", path = "../opentelemetry", features = ["logs"] }
opentelemetry_sdk = { version = "0.27", path = "../opentelemetry-sdk", features = ["logs"] }
tracing = { workspace = true, features = ["std"]}
tracing-core = { workspace = true }
tracing-log = { version = "0.2", optional = true }
tracing-subscriber = { workspace = true, features = ["registry", "std"] }
tracing-opentelemetry = { version = "0.28" }

[dev-dependencies]
log = { workspace = true }
Expand Down
88 changes: 81 additions & 7 deletions opentelemetry-appender-tracing/src/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ use tracing_core::Level;
use tracing_core::Metadata;
#[cfg(feature = "experimental_metadata_attributes")]
use tracing_log::NormalizeEvent;
use tracing_subscriber::Layer;
use tracing_opentelemetry::OtelData;
use tracing_subscriber::{registry::LookupSpan, Layer};

const INSTRUMENTATION_LIBRARY_NAME: &str = "opentelemetry-appender-tracing";

Expand Down Expand Up @@ -149,15 +150,11 @@ where

impl<S, P, L> Layer<S> for OpenTelemetryTracingBridge<P, L>
where
S: tracing::Subscriber,
S: tracing::Subscriber + for<'a> LookupSpan<'a>,
P: LoggerProvider<Logger = L> + Send + Sync + 'static,
L: Logger + Send + Sync + 'static,
{
fn on_event(
&self,
event: &tracing::Event<'_>,
_ctx: tracing_subscriber::layer::Context<'_, S>,
) {
fn on_event(&self, event: &tracing::Event<'_>, ctx: tracing_subscriber::layer::Context<'_, S>) {
#[cfg(feature = "experimental_metadata_attributes")]
let normalized_meta = event.normalized_metadata();

Expand All @@ -180,6 +177,24 @@ where
// Visit fields.
event.record(&mut visitor);

if let Some(span) = ctx.event_span(event) {
let opt_span_id = span
.extensions()
.get::<OtelData>()
.and_then(|otd| otd.builder.span_id);

let opt_trace_id = span.scope().last().and_then(|root_span| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to look for the root span to get the trace_id here? Shouldn't the current span have the trace_id set as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typically the current span won't have the span id explicitly set on it, so it won't actually store the trace id itself, instead it will be inherited when the span is built for sending.

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 suppose going straight to the root span wouldn't support an explicitly set trace id on a non-root span. Is there a non-contrived example of when this might be done that I should add a test for and support? Or should this just be left as a limitation to avoid extra code and runtime overhead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at the listener code and yeah, it's only root spans that get the trace_id set in the builder. Someone would have to manually override that and set a trace_id on the builder to get that behavior.

Still, I think that we should pull out the trace_id from the builder and use that if it's available, or fall back to the root one.

root_span
.extensions()
.get::<OtelData>()
.and_then(|otd| otd.builder.trace_id)
});

if let Some((trace_id, span_id)) = opt_trace_id.zip(opt_span_id) {
log_record.set_trace_context(trace_id, span_id, None);
}
}

//emit record
self.logger.emit(log_record);
}
Expand Down Expand Up @@ -217,6 +232,7 @@ mod tests {
use opentelemetry_sdk::export::logs::{LogBatch, LogExporter};
use opentelemetry_sdk::logs::{LogRecord, LogResult, LoggerProvider};
use opentelemetry_sdk::testing::logs::InMemoryLogExporter;
use opentelemetry_sdk::testing::trace::InMemorySpanExporterBuilder;
use opentelemetry_sdk::trace::{Sampler, TracerProvider};
use tracing::{error, warn};
use tracing_subscriber::prelude::__tracing_subscriber_SubscriberExt;
Expand Down Expand Up @@ -491,6 +507,64 @@ mod tests {
}
}

#[test]
fn tracing_appender_inside_tracing_crate_context() {
// Arrange
let exporter: InMemoryLogExporter = InMemoryLogExporter::default();
let logger_provider = LoggerProvider::builder()
.with_simple_exporter(exporter.clone())
.build();

// setup tracing layer to compare trace/span IDs against
let span_exporter = InMemorySpanExporterBuilder::new().build();
let tracer_provider = TracerProvider::builder()
.with_simple_exporter(span_exporter.clone())
.build();
let tracer = tracer_provider.tracer("test-tracer");

let level_filter = tracing_subscriber::filter::LevelFilter::INFO;
let log_layer =
layer::OpenTelemetryTracingBridge::new(&logger_provider).with_filter(level_filter);

let subscriber = tracing_subscriber::registry()
.with(log_layer)
.with(tracing_opentelemetry::layer().with_tracer(tracer));

// Avoiding global subscriber.init() as that does not play well with unit tests.
let _guard = tracing::subscriber::set_default(subscriber);

// Act
tracing::info_span!("outer-span").in_scope(|| {
error!("first-event");

tracing::info_span!("inner-span").in_scope(|| {
error!("second-event");
});
});

logger_provider.force_flush();

let logs = exporter.get_emitted_logs().expect("No emitted logs");
assert_eq!(logs.len(), 2);

let spans = span_exporter.get_finished_spans().unwrap();
assert_eq!(spans.len(), 2);

let trace_id = spans[0].span_context.trace_id();
assert_eq!(trace_id, spans[1].span_context.trace_id());
let inner_span_id = spans[0].span_context.span_id();
let outer_span_id = spans[1].span_context.span_id();
assert_eq!(outer_span_id, spans[0].parent_span_id);

let trace_ctx0 = logs[0].record.trace_context().unwrap();
let trace_ctx1 = logs[1].record.trace_context().unwrap();

assert_eq!(trace_ctx0.trace_id, trace_id);
assert_eq!(trace_ctx1.trace_id, trace_id);
assert_eq!(trace_ctx0.span_id, outer_span_id);
assert_eq!(trace_ctx1.span_id, inner_span_id);
}

#[test]
fn tracing_appender_standalone_with_tracing_log() {
// Arrange
Expand Down