-
Notifications
You must be signed in to change notification settings - Fork 578
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
Recommend installing opentelemetry-instrumentation-asgi
under certain situations (opentelemetry-instrumentation-django)
#2408
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR, care to add a test too? |
Is it essential to add test code to add a mere line of logging? |
It's not essential of course, but it'll help :) |
If a test code is not essential, please review as is. |
I don't think we should log such a warning more than once. Please use warnings.warn instead. |
If developers are using Moreover, if multiple workers run using gunicorn, the warnings will be logged more than once even if we use |
This will warn at each request not for each worker |
Of course, I know. |
@@ -197,6 +197,9 @@ def process_request(self, request): | |||
|
|||
is_asgi_request = _is_asgi_request(request) | |||
if not _is_asgi_supported and is_asgi_request: |
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.
Maybe something like this?
if not _is_asgi_supported and is_asgi_request: | |
self._unsupported_asgi_warned = False | |
... | |
if not _is_asgi_supported and is_asgi_request and not self._unsupported_asgi_warned: | |
... | |
self._unsupported_asgi_warned = True |
Could be a good idea, or a terrible one 🤷
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.
Added a comment
opentelemetry-instrumentation-asgi
under certain situations (opentelemetry-instrumentation-django)
@srikanthccv @ocelotl I fixed to log a warning only once. |
@@ -197,6 +209,7 @@ def process_request(self, request): | |||
|
|||
is_asgi_request = _is_asgi_request(request) | |||
if not _is_asgi_supported and is_asgi_request: | |||
_OnceLogger.log_warning_asgi_instrumentation() |
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.
Please add a test case, see this.
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.
Is there already a test for a pattern where the request type is ASGIRequest
and opentelemetry-instrumentation-asgi
is not installed?
Description
When I added the
opentelemetry-instrumentation-django
package and started the server withpython manage.py runserver
callingDjangoInstrumentor().instrument()
in manage.py, inbound HTTP request spans were correctly created.However, when using Django with Gunicorn, there was a problem: inbound HTTP request spans were not created.
To solve this problem, I spent a lot of time reading the
opentelemetry-instrumentation-django
code and found that addingopentelemetry-instrumentation-asgi
would solve this problem. I have arrived at the solution.The current implementation silently turns off instrumentation in certain situations, such as using Gunicorn.
Instrumentation library users will expect the instrumentation to work correctly if the call to
DjangoInstrumentor().instrument()
succeeds and there are no warnings in the logs. Also, the documentation does not mention the need to addopentelemetry-instrumentation-asgi
.This wastes a lot of time for many developers. Therefore, I have added a log that shows how to deal with this problem instead of silently disabling instrumentation when it occurs.
Related: #2043 #2296
Type of change
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.