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

Add http.base_url tag to Rack #327

Merged
merged 2 commits into from
Jan 27, 2018
Merged

Add http.base_url tag to Rack #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a876ea
Added: http.base_url tag to Rack.
delner Jan 24, 2018
a3f6704
[rack] reducing Rack middleware `call` complexity
Jan 27, 2018
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
68 changes: 41 additions & 27 deletions lib/ddtrace/contrib/rack/middlewares.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,14 @@ def initialize(app)
def call(env)
# retrieve integration settings
tracer = Datadog.configuration[:rack][:tracer]
service = Datadog.configuration[:rack][:service_name]
distributed_tracing = Datadog.configuration[:rack][:distributed_tracing]
Copy link
Contributor

Choose a reason for hiding this comment

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

Removed this variables since they're used once (reducing the method's length)


trace_options = {
service: service,
service: Datadog.configuration[:rack][:service_name],
resource: nil,
span_type: Datadog::Ext::HTTP::TYPE
}

if distributed_tracing
if Datadog.configuration[:rack][:distributed_tracing]
context = HTTPPropagator.extract(env)
tracer.provider.context = context if context.trace_id
end
Expand All @@ -56,35 +54,13 @@ def call(env)
request_span.set_error(e) unless request_span.nil?
raise e
ensure
# the source of truth in Rack is the PATH_INFO key that holds the
# URL for the current request; some framework may override that
# value, especially during exception handling and because of that
# we prefer using the `REQUEST_URI` if this is available.
# NOTE: `REQUEST_URI` is Rails specific and may not apply for other frameworks
url = env['REQUEST_URI'] || env['PATH_INFO']

# Rack is a really low level interface and it doesn't provide any
# advanced functionality like routers. Because of that, we assume that
# the underlying framework or application has more knowledge about
# the result for this request; `resource` and `tags` are expected to
# be set in another level but if they're missing, reasonable defaults
# are used.
request_span.resource ||= resource_name_for(env, status)
if request_span.get_tag(Datadog::Ext::HTTP::METHOD).nil?
request_span.set_tag(Datadog::Ext::HTTP::METHOD, env['REQUEST_METHOD'])
end
if request_span.get_tag(Datadog::Ext::HTTP::URL).nil?
request_span.set_tag(Datadog::Ext::HTTP::URL, url)
end
if request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE).nil? && status
request_span.set_tag(Datadog::Ext::HTTP::STATUS_CODE, status)
end

# detect if the status code is a 5xx and flag the request span as an error
# unless it has been already set by the underlying framework
if status.to_s.start_with?('5') && request_span.status.zero?
request_span.status = 1
end
set_request_tags!(request_span, env, status, headers, response)
Copy link
Contributor

Choose a reason for hiding this comment

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

to reduce the method Abc complexity, I've moved these actions in a separated method that updates in place request_span. It's just moving, though the logic is still the same.


# ensure the request_span is finished and the context reset;
# this assumes that the Rack middleware creates a root span
Expand All @@ -103,6 +79,44 @@ def resource_name_for(env, status)
"#{env['REQUEST_METHOD']} #{status}".strip
end
end

def set_request_tags!(request_span, env, status, headers, response)
# the source of truth in Rack is the PATH_INFO key that holds the
# URL for the current request; some framework may override that
# value, especially during exception handling and because of that
# we prefer using the `REQUEST_URI` if this is available.
# NOTE: `REQUEST_URI` is Rails specific and may not apply for other frameworks
url = env['REQUEST_URI'] || env['PATH_INFO']

request_span.resource ||= resource_name_for(env, status)
if request_span.get_tag(Datadog::Ext::HTTP::METHOD).nil?
request_span.set_tag(Datadog::Ext::HTTP::METHOD, env['REQUEST_METHOD'])
end
if request_span.get_tag(Datadog::Ext::HTTP::URL).nil?
request_span.set_tag(Datadog::Ext::HTTP::URL, url)
end
if request_span.get_tag(Datadog::Ext::HTTP::BASE_URL).nil?
request_obj = ::Rack::Request.new(env)

base_url = if request_obj.respond_to?(:base_url)
request_obj.base_url
else
# Compatibility for older Rack versions
request_obj.url.chomp(request_obj.fullpath)
end

request_span.set_tag(Datadog::Ext::HTTP::BASE_URL, base_url)
end
if request_span.get_tag(Datadog::Ext::HTTP::STATUS_CODE).nil? && status
request_span.set_tag(Datadog::Ext::HTTP::STATUS_CODE, status)
end

# detect if the status code is a 5xx and flag the request span as an error
# unless it has been already set by the underlying framework
if status.to_s.start_with?('5') && request_span.status.zero?
request_span.status = 1
end
end
end
end
end
Expand Down
1 change: 1 addition & 0 deletions lib/ddtrace/ext/http.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module HTTP
TYPE = 'http'.freeze
TEMPLATE = 'template'.freeze
URL = 'http.url'.freeze
BASE_URL = 'http.base_url'.freeze
METHOD = 'http.method'.freeze
STATUS_CODE = 'http.status_code'.freeze
ERROR_RANGE = 500...600
Expand Down
12 changes: 12 additions & 0 deletions test/contrib/rack/middleware_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ def test_request_middleware_get
assert_equal('GET', span.get_tag('http.method'))
assert_equal('200', span.get_tag('http.status_code'))
assert_equal('/success/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(0, span.status)
assert_nil(span.parent)
end
Expand All @@ -38,6 +39,7 @@ def test_request_middleware_post
assert_equal('POST', span.get_tag('http.method'))
assert_equal('200', span.get_tag('http.status_code'))
assert_equal('/success/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(0, span.status)
assert_nil(span.parent)
end
Expand All @@ -58,6 +60,7 @@ def test_request_middleware_routes
assert_equal('GET', span.get_tag('http.method'))
assert_equal('200', span.get_tag('http.status_code'))
assert_equal('/success/100', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(0, span.status)
assert_nil(span.parent)
end
Expand All @@ -78,6 +81,7 @@ def test_request_middleware_missing_application
assert_equal('GET', span.get_tag('http.method'))
assert_equal('404', span.get_tag('http.status_code'))
assert_equal('/not/exists/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(0, span.status)
assert_nil(span.parent)
end
Expand All @@ -99,6 +103,7 @@ def test_request_middleware_bad_request
assert_equal('GET', span.get_tag('http.method'))
assert_equal('400', span.get_tag('http.status_code'))
assert_equal('/failure/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(0, span.status)
assert_nil(span.parent)
end
Expand All @@ -121,6 +126,7 @@ def test_request_middleware_exception
assert_equal('GET', span.get_tag('http.method'))
assert_nil(span.get_tag('http.status_code'))
assert_equal('/exception/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal('StandardError', span.get_tag('error.type'))
assert_equal('Unable to process the request', span.get_tag('error.msg'))
refute_nil(span.get_tag('error.stack'))
Expand All @@ -144,6 +150,7 @@ def test_request_middleware_rack_app
assert_equal('GET_V2', span.get_tag('http.method'))
assert_equal('201', span.get_tag('http.status_code'))
assert_equal('/app/static/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(0, span.status)
assert_nil(span.parent)
end
Expand All @@ -165,6 +172,7 @@ def test_request_middleware_500
assert_equal('GET', span.get_tag('http.method'))
assert_equal('500', span.get_tag('http.status_code'))
assert_equal('/500/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_nil(span.get_tag('error.stack'))
assert_equal(1, span.status)
assert_nil(span.parent)
Expand All @@ -187,6 +195,7 @@ def test_request_middleware_500_handled
assert_equal('GET', span.get_tag('http.method'))
assert_equal('500', span.get_tag('http.status_code'))
assert_equal('/app/500/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(1, span.status)
assert_equal('Handled exception', span.get_tag('error.stack'))
assert_nil(span.parent)
Expand All @@ -210,6 +219,7 @@ def test_request_middleware_500_handled_without_status
assert_equal('GET', span.get_tag('http.method'))
assert_equal('500', span.get_tag('http.status_code'))
assert_equal('/app/500/no_status/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(1, span.status)
assert_equal('Handled exception', span.get_tag('error.stack'))
assert_nil(span.parent)
Expand All @@ -233,6 +243,7 @@ def test_request_middleware_non_standard_error
assert_equal('GET', span.get_tag('http.method'))
assert_nil(span.get_tag('http.status_code'))
assert_equal('/nomemory/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal('NoMemoryError', span.get_tag('error.type'))
assert_equal('Non-standard error', span.get_tag('error.msg'))
refute_nil(span.get_tag('error.stack'))
Expand Down Expand Up @@ -268,6 +279,7 @@ def test_request_middleware_custom_service
assert_equal('GET', span.get_tag('http.method'))
assert_equal('200', span.get_tag('http.status_code'))
assert_equal('/success/', span.get_tag('http.url'))
assert_equal('http://example.org', span.get_tag('http.base_url'))
assert_equal(0, span.status)
assert_nil(span.parent)
end
Expand Down