Skip to content

Commit

Permalink
Fix NoMethodError in sinatra integration when Tracer middleware is mi…
Browse files Browse the repository at this point in the history
…ssing

This fixes a regression introduced in version 0.52.0 by #1628.

When a Sinatra modular application being traced is missing the
`Datadog::Contrib::Sinatra::Tracer` middleware
(as documented in
<https://docs.datadoghq.com/tracing/setup_overview/setup/ruby/#sinatra>),
no Sinatra request span is ever created (only a route span).

Because we didn't properly account for this in `#datadog_span`, the
following `NoMethodError` was triggered:

```
NoMethodError: undefined method `[]' for nil:NilClass
  dd-trace-rb/lib/ddtrace/contrib/sinatra/env.rb:13:in `datadog_span'
  dd-trace-rb/lib/ddtrace/contrib/sinatra/tracer.rb:122:in `block in route_eval'
  dd-trace-rb/lib/ddtrace/tracer.rb:283:in `trace'
  dd-trace-rb/lib/ddtrace/contrib/sinatra/tracer.rb:105:in `route_eval'
  ruby-2.7.4/gems/sinatra-2.1.0/lib/sinatra/base.rb:1013:in `block (2 levels) in route!'
```

I suspect this is the same issue as was reported in #1643.

Because this configuration is actually not desirable -- spans reported
will be missing crucial request information, as discussed in #1643 --
I've also added a warning to hopefully steer users in the right
direction.
  • Loading branch information
ivoanjo committed Aug 11, 2021
1 parent f7431ce commit 1005755
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 3 deletions.
3 changes: 2 additions & 1 deletion lib/ddtrace/contrib/sinatra/env.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ module Env
module_function

def datadog_span(env, app)
env[Ext::RACK_ENV_REQUEST_SPAN][app]
request_span = env[Ext::RACK_ENV_REQUEST_SPAN]
request_span && request_span[app]
end

def set_datadog_span(env, app, span)
Expand Down
17 changes: 15 additions & 2 deletions lib/ddtrace/contrib/sinatra/tracer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
require 'ddtrace/ext/errors'
require 'ddtrace/ext/http'
require 'ddtrace/propagation/http_propagator'

require 'ddtrace/utils/only_once'
require 'ddtrace/contrib/sinatra/ext'
require 'ddtrace/contrib/sinatra/tracer_middleware'
require 'ddtrace/contrib/sinatra/env'
Expand Down Expand Up @@ -77,6 +77,9 @@ def self.registered(app)

# Method overrides for Sinatra::Base
module Base
MISSING_REQUEST_SPAN_ONLY_ONCE = Datadog::Utils::OnlyOnce.new
private_constant :MISSING_REQUEST_SPAN_ONLY_ONCE

def render(engine, data, *)
tracer = Datadog.configuration[:sinatra][:tracer]
return super unless tracer.enabled
Expand Down Expand Up @@ -121,8 +124,18 @@ def route_eval
else
Sinatra::Env.datadog_span(env, self.class)
end
if sinatra_request_span # DEV: Is it possible for sinatra_request_span to ever be nil here?
if sinatra_request_span
sinatra_request_span.resource = span.resource
else
MISSING_REQUEST_SPAN_ONLY_ONCE.run do
Datadog.logger.warn do
'Sinatra integration is misconfigured, reported traces will be missing request metadata ' \
'such as path and HTTP status code. ' \
'Did you forget to add `register Datadog::Contrib::Sinatra::Tracer` to your ' \
'`Sinatra::Base` subclass? ' \
'See <https://docs.datadoghq.com/tracing/setup_overview/setup/ruby/#sinatra> for more details.'
end
end
end

Contrib::Analytics.set_measured(span)
Expand Down
29 changes: 29 additions & 0 deletions spec/ddtrace/contrib/sinatra/tracer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,35 @@
end
end
end

context 'when modular app does not register the Datadog::Contrib::Sinatra::Tracer middleware' do
let(:sinatra_app) do
sinatra_routes = self.sinatra_routes
stub_const('App', Class.new(Sinatra::Base) do
instance_exec(&sinatra_routes)
end)
end

subject(:response) { get url }

before do
allow(Datadog.logger).to receive(:warn)
Datadog::Contrib::Sinatra::Tracer::Base
.const_get('MISSING_REQUEST_SPAN_ONLY_ONCE').send(:reset_ran_once_state_for_tests)
end

it { is_expected.to be_ok }

it 'logs a warning' do
expect(Datadog.logger).to receive(:warn) do |&message|
expect(message.call).to include 'Sinatra integration is misconfigured'
end

# NOTE: We actually need to check that the request finished ok, as sinatra may otherwise "swallow" an RSpec
# exception and thus the test will appear to pass because the RSpec exception doesn't propagate out
is_expected.to be_ok
end
end
end

RSpec::Matchers.define :be_request_span do |opts = {}|
Expand Down

0 comments on commit 1005755

Please sign in to comment.