-
Notifications
You must be signed in to change notification settings - Fork 375
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
Use full URL in http.url
tag
#2265
Changes from all commits
3b2c316
f439d0e
ec68ba1
7d4c742
337ecb7
a741230
5bbbffb
a5ff117
8c848ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,18 +123,6 @@ def call(env) | |
# rubocop:disable Metrics/PerceivedComplexity | ||
# rubocop:disable Metrics/MethodLength | ||
def set_request_tags!(trace, request_span, env, status, headers, response, original_env) | ||
# http://www.rubydoc.info/github/rack/rack/file/SPEC | ||
# The source of truth in Rack is the PATH_INFO key that holds the | ||
# URL for the current request; but some frameworks may override that | ||
# value, especially during exception handling. | ||
# | ||
# Because of this, we prefer to use REQUEST_URI, if available, which is the | ||
# relative path + query string, and doesn't mutate. | ||
# | ||
# REQUEST_URI is only available depending on what web server is running though. | ||
# So when its not available, we want the original, unmutated PATH_INFO, which | ||
# is just the relative path without query strings. | ||
url = env['REQUEST_URI'] || original_env['PATH_INFO'] | ||
request_header_collection = Header::RequestHeaderCollection.new(env) | ||
request_headers_tags = parse_request_headers(request_header_collection) | ||
response_headers_tags = parse_response_headers(headers || {}) | ||
|
@@ -176,14 +164,35 @@ def set_request_tags!(trace, request_span, env, status, headers, response, origi | |
request_span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_METHOD, env['REQUEST_METHOD']) | ||
end | ||
|
||
url = parse_url(env, original_env) | ||
|
||
if request_span.get_tag(Tracing::Metadata::Ext::HTTP::TAG_URL).nil? | ||
options = configuration[:quantize] | ||
options = configuration[:quantize] || {} | ||
|
||
# Quantization::HTTP.url base defaults to :show, but we are transitioning | ||
options[:base] ||= :exclude | ||
Comment on lines
+170
to
+173
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has to be done here, as doing it as a config default would be silently overridden when Probably a general problem with such nested configuration keys. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand it correctly, this will be removed in the next major release, is that right? If so, I suggest writing them down in the comment, so we can find it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, can you create a new file named
We've been meaning to do it, and it seems like your PR wants to have a breaking change in the next release. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this, I suggest adding the comment in this line with the prefix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good points! Will do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'll do that in another PR, along with this suggestion: #2283 (comment) |
||
|
||
request_span.set_tag( | ||
Tracing::Metadata::Ext::HTTP::TAG_URL, | ||
Contrib::Utils::Quantization::HTTP.url(url, options) | ||
) | ||
end | ||
|
||
if request_span.get_tag(Tracing::Metadata::Ext::HTTP::TAG_BASE_URL).nil? | ||
options = configuration[:quantize] | ||
|
||
unless options[:base] == :show | ||
base_url = Contrib::Utils::Quantization::HTTP.base_url(url) | ||
|
||
unless base_url.empty? | ||
request_span.set_tag( | ||
Tracing::Metadata::Ext::HTTP::TAG_BASE_URL, | ||
base_url | ||
) | ||
end | ||
end | ||
end | ||
|
||
if request_span.get_tag(Tracing::Metadata::Ext::HTTP::TAG_CLIENT_IP).nil? | ||
Tracing::ClientIp.set_client_ip_tag( | ||
request_span, | ||
|
@@ -192,19 +201,6 @@ def set_request_tags!(trace, request_span, env, status, headers, response, origi | |
) | ||
end | ||
|
||
if request_span.get_tag(Tracing::Metadata::Ext::HTTP::TAG_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(Tracing::Metadata::Ext::HTTP::TAG_BASE_URL, base_url) | ||
end | ||
|
||
if request_span.get_tag(Tracing::Metadata::Ext::HTTP::TAG_STATUS_CODE).nil? && status | ||
request_span.set_tag(Tracing::Metadata::Ext::HTTP::TAG_STATUS_CODE, status) | ||
end | ||
|
@@ -238,6 +234,46 @@ def configuration | |
Datadog.configuration.tracing[:rack] | ||
end | ||
|
||
def parse_url(env, original_env) | ||
request_obj = ::Rack::Request.new(env) | ||
|
||
# scheme, host, and port | ||
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 | ||
|
||
# https://github.com/rack/rack/blob/main/SPEC.rdoc | ||
# | ||
# The source of truth in Rack is the PATH_INFO key that holds the | ||
# URL for the current request; but some frameworks may override that | ||
# value, especially during exception handling. | ||
# | ||
# Because of this, we prefer to use REQUEST_URI, if available, which is the | ||
# relative path + query string, and doesn't mutate. | ||
# | ||
# REQUEST_URI is only available depending on what web server is running though. | ||
# So when its not available, we want the original, unmutated PATH_INFO, which | ||
# is just the relative path without query strings. | ||
# | ||
# SCRIPT_NAME is the first part of the request URL path, so that | ||
# the application can know its virtual location. It should be | ||
# prepended to PATH_INFO to reflect the correct user visible path. | ||
request_uri = env['REQUEST_URI'].to_s | ||
fullpath = if request_uri.empty? | ||
query_string = original_env['QUERY_STRING'].to_s | ||
path = original_env['SCRIPT_NAME'].to_s + original_env['PATH_INFO'].to_s | ||
|
||
query_string.empty? ? path : "#{path}?#{query_string}" | ||
else | ||
request_uri | ||
end | ||
|
||
base_url + fullpath | ||
end | ||
|
||
def parse_user_agent_header(headers) | ||
headers.get(Tracing::Metadata::Ext::HTTP::HEADER_USER_AGENT) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ironically, the doc here was consistent with the internal
Quantization::HTTP.url
but not with the user-facing Rack integrationhttp.url
tagging behaviour!