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

Rack: Fix missing active trace #3420

Conversation

TonyCTHsu
Copy link
Contributor

What does this PR do?

closes #3389

When tracing and distributed_tracing both disabled, The reference of Datadog::Tracing.active_trace in rack
instrumentation returns nil.

Causing

NoMethodError: undefined method `resource_override?' for nil:NilClass

@TonyCTHsu TonyCTHsu requested a review from a team as a code owner January 30, 2024 16:59
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Jan 30, 2024
@TonyCTHsu TonyCTHsu added this to the 1.20.0 milestone Jan 30, 2024

# When tracing and distributed tracing are both disabled, `.active_trace` will be `nil`,
# Return a null object to continue operation
request_trace = Tracing.active_trace || TraceOperation.new
Copy link
Contributor

Choose a reason for hiding this comment

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

The exception happens in the call to set_request_tags! made in this method. Wouldn't it be better to expand the condition on line 72 to check if tracing is disabled, and if so, skip the remainder of the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 @p-datadog , your suggested alternative will definitely also mitigate this error.

The reason for my change is because it follows the same null object pattern from our API .trace(..) when invoke with a block (Yeah, it is kinda flawed that the blockless form only return a span_operation, hence the trace_operation is only accessible through .active_trace )

I think the best solution for this would be to instrument with a block, this would by default yield null objects for both span_operation and trace_operation, that would made all those conditionals obsolete.

However, that is a much bigger change for this already messy instrumentation.

@TonyCTHsu TonyCTHsu merged commit 7753aba into master Feb 1, 2024
218 of 219 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/fix-rack-disabled-with-tracing-without-distributed-tracing branch February 1, 2024 09:24
@TonyCTHsu TonyCTHsu mentioned this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rails instrumentation fails with distributed_tracing=false
4 participants