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

Copy parent trace_id from parent span #80

Open
wants to merge 1 commit into
base: v0.1.x
Choose a base branch
from

Conversation

maximebedard
Copy link

Motivation

I ran into an issue where the trace_id of the parent span is not copied into the SpanContext of the current span. This can be especially problematic when downstream APIs rely on the OtelData structure to extract the trace_id and span_id.

I believe the test was incorrect as it was only checking that the value was left unchanged in the parent span, not that the trace_id of the current span was set.

I believe this is related to #77.

Solution

I'm copying the trace_id from the parent_cx.span when the span has a parent context. Otherwise we generate a new trace_id. I've updated the test_tracer to generate sequential {TraceId,SpanId} to potentially avoid comparing against {TraceId,SpanId}::INVALID.

Thanks!

@jtescher
Copy link
Collaborator

jtescher commented Jan 8, 2024

Hm could you elaborate more on the case where parent span ids are not currently copied? Currently the trace id is intentionally only tracked for cases where there is no active parent so it can be used for pre-sampling.

@maximebedard
Copy link
Author

TIL, I didn't know that's how sampling was handled internally. I haven't looked to much in depth how it's implemented, but I suppose the trace_id is generated when the span is closed instead of when it's created? If that's the case, I'm not really sure how I could solve my problem.

The use-case I currently have is to copy the trace_id and span_id on log records emitted by using the opentelemetry-appender-tracing. I originally had this PR opened that extracted the trace_id and span_id from the OtelData context and would set the trace_context based on the span where the log even was emited (See https://github.com/open-telemetry/opentelemetry-rust/pull/1394/files#diff-e9033bd5189bc3b88a31d73e8dfd111a281b76885313fc2c8421b4f9332b452bR153). This allows me to visualize logs and correlate them with their respective spans, which is especially useful when using tools like datadog.

I had to close the original PR due to a circular dependency issue, but I'm using my own version of the opentelemetry-appender-tracing that uses published versions of the tracing-opentelementry and opentelemetry-rust crates.

@jtescher
Copy link
Collaborator

jtescher commented Jan 8, 2024

So the main issue is ordering around calls to OpenTelemetrySpanExt::set_parent for spans, e.g. if you were to start a new span, then emit a log record (it would get the current span's trace id A) then you call set_parent with trace_id B, and emit another record, when the span ends it will be exported with trace id B, and your first log will be associated with A will be confusing. For this reason calls for span contexts go through the OpenTelemetrySpanExt::context method which generates a stable trace id if there isn't a parent. If you wanted to get a trace id for your logs you could use the same trick, but you'd have to ensure that there aren't subsequent calls to update the parent context.

@maximebedard
Copy link
Author

Could a more reliable approach be to buffer the LogRecords, this way we could retrieve a stable trace_id and export them without having to consider calls to set_parent?

@jtescher
Copy link
Collaborator

@maximebedard maybe, you could explore that path and see what tradeoffs you would have to make. Potentially unbounded buffering as logs within a given span aren't capped, also delaying log export until the parent is closed be undesirable. Alternatively we could consider a special field type solution where if a trace id has been set there it can't be re assigned, and there may be other possible alternative solutions to explore as well.

@hdost
Copy link
Contributor

hdost commented Feb 18, 2024

@jtescher the related opentelemetry rust change open-telemetry/opentelemetry-rust#1394

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.

3 participants