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.

Sign up for GitHub

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

Jump to bottom

Rack: Fix missing active trace #3420

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Rack: Fix missing active trace #3420

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52529eb
Failing test with both tracing and distributed_tracing both disabled
TonyCTHsu Jan 30, 2024
0ad89e9
Fix with null object
TonyCTHsu Jan 30, 2024
37922e8
Fix flaky spec by resetting configuration properly
TonyCTHsu Jan 30, 2024
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
6 changes: 5 additions & 1 deletion lib/datadog/tracing/contrib/rack/middlewares.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ def call(env)
# we must ensure that the span `resource` is set later
request_span = Tracing.trace(Ext::SPAN_REQUEST, **trace_options)
request_span.resource = nil
request_trace = Tracing.active_trace

# 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.


env[Ext::RACK_ENV_REQUEST_SPAN] = request_span

# Copy the original env, before the rest of the stack executes.
Expand Down
43 changes: 43 additions & 0 deletions spec/datadog/tracing/contrib/rack/disabled_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
require 'datadog/tracing/contrib/support/spec_helper'
require 'rack/test'
require 'rack'
require 'ddtrace'
require 'datadog/tracing/contrib/rack/middlewares'

RSpec.describe 'Rack integration tests' do
include Rack::Test::Methods

before do
Datadog.configure do |c|
c.tracing.enabled = false
c.tracing.instrument :rack, distributed_tracing: false
end
end

after do
Datadog.configuration.reset!
end

context 'for an application' do
let(:app) do
Rack::Builder.new do
use Datadog::Tracing::Contrib::Rack::TraceMiddleware
map '/success/' do
run(proc { |_env| [200, { 'Content-Type' => 'text/html' }, ['OK']] })
end
end.to_app
end

context 'with a basic route' do
describe 'GET request' do
it do
response = get 'success'

expect(response).to be_ok

expect(spans).to have(0).items
end
end
end
end
end
Loading