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

[fix] rack middleware close trace via body proxy (Rack::CommonLogger compatibility) #1746

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions lib/ddtrace/contrib/delayed_job/plugin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module DelayedJob
# DelayedJob plugin that instruments invoke_job hook
class Plugin < Delayed::Plugin
def self.instrument_invoke(job, &block)
return block.call(job) unless tracer && tracer.enabled
return yield(job) unless tracer && tracer.enabled

tracer.trace(Ext::SPAN_JOB, service: configuration[:service_name], resource: job_name(job),
on_error: configuration[:error_handler]) do |span|
Expand All @@ -29,7 +29,7 @@ def self.instrument_invoke(job, &block)
end

def self.instrument_enqueue(job, &block)
return block.call(job) unless tracer && tracer.enabled
return yield(job) unless tracer && tracer.enabled

tracer.trace(Ext::SPAN_ENQUEUE, service: configuration[:client_service_name], resource: job_name(job)) do |span|
set_sample_rate(span)
Expand Down
24 changes: 21 additions & 3 deletions lib/ddtrace/contrib/rack/middlewares.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,21 @@ def call(env)

# call the rest of the stack
status, headers, response = @app.call(env)
[status, headers, response]

# some middleware like Rack::CommonLogger wraps body via Rack::BodyProxy
# Rack::BodyProxy is a correct mechanism to finish span after all middleware call
# so Rack::CommonLogger will have trace_id & span_id tags

body = if defined?(::Rack::BodyProxy)
::Rack::BodyProxy.new(response) do
close_trace(request_span, frontend_span, env, status, headers, response, original_env, tracer)
end
else
close_trace(request_span, frontend_span, env, status, headers, response, original_env, tracer)
response
end

[status, headers, body]

# rubocop:disable Lint/RescueException
# Here we really want to catch *any* exception, not only StandardError,
Expand All @@ -98,9 +112,13 @@ def call(env)
# catch exceptions that may be raised in the middleware chain
# Note: if a middleware catches an Exception without re raising,
# the Exception cannot be recorded here.
request_span.set_error(e) unless request_span.nil?
request_span.set_error(e) if request_span

close_trace(request_span, frontend_span, env, status, headers, response, original_env, tracer)
raise e
ensure
end

def close_trace(request_span, frontend_span, env, status, headers, response, original_env, tracer)
if request_span
# Rack is a really low level interface and it doesn't provide any
# advanced functionality like routers. Because of that, we assume that
Expand Down
1 change: 1 addition & 0 deletions lib/ddtrace/contrib/rack/patcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ def target_version

def patch
# Patch middleware
require 'rack'
require_relative 'middlewares'
end
end
Expand Down
1 change: 1 addition & 0 deletions sorbet/rbi/todo.rbi

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.