-
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
Bugfix django instrumentation #1309
Bugfix django instrumentation #1309
Conversation
…rect resolver we need use request.path to replace request.get_full_path()
Does this change strip off the query string from span name? |
no no no if request have query_string. The django resolve not match route so span name only "HTTP {http_method}", we want span name is django view_name or func_name or route |
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 catch! Can you please add a change to the changelog? The this is good to go!
@@ -3,6 +3,7 @@ | |||
## Unreleased | |||
|
|||
- Django instrumentation is now enabled by default but can be disabled by setting `OTEL_PYTHON_DJANGO_INSTRUMENT` to `False` ([#1239](https://github.com/open-telemetry/opentelemetry-python/pull/1239)) | |||
- Bugfix use request.path replace request.get_full_path(). It will get correct span name ([#1309](https://github.com/open-telemetry/opentelemetry-python/pull/1309#)) |
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.
Typically with changelogs, you should try to have information that's relevant to the user, so mainly behavior. The details aren't really helpful to most.
In this case, it's sufficient to say "span name resolves correctly for paths that also require a query parameter to match".
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.
Thank you for your suggestion, I will pay attention to it later
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.
Great, thanks!
* Basic node * Tests and functions * DefaultURL * Add JSON tests * Test rename * Refactor a lot * Restored those files * Variable anme * Fixed strings * default grpc, collector name'
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.
👍
Description
Fix: Django instrumentation if request have query_string will not correct resolver we need use request.path to replace request.get_full_path()
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: