-
Notifications
You must be signed in to change notification settings - Fork 623
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
Add capture of http.route to DjangoInstrumentor middleware #1226
Changes from 8 commits
12da4f6
7362f73
e3979de
f070259
9190a7d
5f7927b
d919de2
3bd216d
a5889bb
aa4e35b
afadb66
ad9dfa5
7348240
9170599
b65e384
3f93411
6a5c07f
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 |
---|---|---|
|
@@ -117,6 +117,7 @@ def process_request(self, request): | |
) | ||
for key, value in attributes.items(): | ||
span.set_attribute(key, value) | ||
span.set_attribute("http.path", request.path) | ||
|
||
activation = tracer.use_span(span, end_on_exit=True) | ||
activation.__enter__() | ||
|
@@ -125,6 +126,26 @@ def process_request(self, request): | |
request.META[self._environ_span_key] = span | ||
request.META[self._environ_token] = token | ||
|
||
def process_view(self, request, view_func, *args, **kwargs): | ||
# Process view is executed before the view function, here we get the | ||
# route template from request.resolver_match. It is not set yet in process_request | ||
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")): | ||
return | ||
|
||
if ( | ||
self._environ_activation_key in request.META.keys() | ||
and self._environ_span_key in request.META.keys() | ||
): | ||
span = request.META[self._environ_span_key] | ||
|
||
if span.is_recording(): | ||
if getattr(request, "resolver_match") and getattr( | ||
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. nit: 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 used getattr because I also don't want to capture the value if it is None. 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. match = getattr(request, "resolved_match")
if match:
route = getattr(match, "route")
if route:
set_attribute() Perhaps use this? I know it's usually not a big deal but we never know where the library will be used and better to reduce attribute look ups if we can in case the code ends up on a very hot path. 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. makes sense... I have not thought about reducing attribute lookups before. Thanks! |
||
request.resolver_match, "route" | ||
): | ||
span.set_attribute( | ||
"http.route", request.resolver_match.route | ||
) | ||
|
||
def process_exception(self, request, exception): | ||
# Django can call this method and process_response later. In order | ||
# to avoid __exit__ and detach from being called twice then, the | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,7 @@ def test_traced_get(self): | |
self.assertEqual( | ||
span.attributes["http.url"], "http://testserver/traced/" | ||
) | ||
self.assertEqual(span.attributes["http.route"], "^traced/") | ||
self.assertEqual(span.attributes["http.scheme"], "http") | ||
self.assertEqual(span.attributes["http.status_code"], 200) | ||
self.assertEqual(span.attributes["http.status_text"], "OK") | ||
|
@@ -121,6 +122,7 @@ def test_traced_post(self): | |
self.assertEqual( | ||
span.attributes["http.url"], "http://testserver/traced/" | ||
) | ||
self.assertEqual(span.attributes["http.route"], "^traced/") | ||
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. would be nice to add a templated route so tests cover that and catch any regressions in future. |
||
self.assertEqual(span.attributes["http.scheme"], "http") | ||
self.assertEqual(span.attributes["http.status_code"], 200) | ||
self.assertEqual(span.attributes["http.status_text"], "OK") | ||
|
@@ -145,6 +147,7 @@ def test_error(self): | |
self.assertEqual( | ||
span.attributes["http.url"], "http://testserver/error/" | ||
) | ||
self.assertEqual(span.attributes["http.route"], "^error/") | ||
self.assertEqual(span.attributes["http.scheme"], "http") | ||
|
||
@patch( | ||
|
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.
This should be
http.target
according to the spec. https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#common-attributesThere 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.
Sound good.
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.
Isn't
http.target
already being populated incollect_request_attributes
?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.
Good point @lzchen! I will remove the setting of the attribute from the django middleware.