-
Notifications
You must be signed in to change notification settings - Fork 599
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
[FLASK] added request and response hook #416
Conversation
|
@NickSulistio |
…y-python-contrib into flask_hooks Updating my branch
…emetry-python-contrib into flask_hooks Get latest version
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py
Outdated
Show resolved
Hide resolved
|
||
FlaskInstrumentor().instrument(otel_request_hook=request_hook, otel_response_hook=response_hook) | ||
# pylint: disable=import-outside-toplevel,reimported,redefined-outer-name | ||
from flask import Flask |
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 flask being imported globally above? why do we need 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.
Hey Owais, so if I don't import Flask here, no span will be created. I'm not sure exactly why this is the case but when I removed that import line, the span was no longer being created.
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.
I think you'll be able to solve it by creating your flask app instance after calling instrument(). So for each test case, first call .instument()
and then create flask app instance.
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.
Hey Owais, if I'm not mistaken, aren't I already calling .instrument() before I import Flask and create the app instance?
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.
I don't see the app being created anywhere in this method. May be it is being created in the super-class in which case it would be created before since super().setUp() is being called before instrument.
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.
Hey Owais, for this test, we're testing the hooks without the App being created. I'm not entirely sure how this is supposed to work, but previously there was a test for TestProgrammaticCustomSpanNameWithoutApp
, which did the same. I believe that @lonewolf3739 may have a better understanding of what this is used for.
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.
@NickSulistio Sorry for getting back late. I think If you do import from flask import Flask
first and next call FlaskInstrumentor().instrument()
then instrumentation doesn't happen because the way patching is done; I remember this was the reason but I may be wrong.
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.
If this is a limitation with the instrumentation itself, I think we can live with it for now and try to improve the instrumentation later.
{"http.target": "/hello/123", "http.route": "/hello/<int:helloid>", "hook_attr":"hello world"} | ||
) | ||
|
||
self.client.get("/hello/123") |
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.
I would also test adding a header to the response and making sure it was seen here.
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.
If you don't mind, could you elaborate on how to do that?
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.
You'd need to update the post response hook and inject headers into the response there. Look at Flask or WSGI docs (which ever object is passed to the response hook). Then you'd capture the result from client.get()
call here and verify the result has the same headers you set in the hook earlier.
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Leighton Chen <[email protected]>
instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py
Outdated
Show resolved
Hide resolved
@ocelotl Would you like to review this PR? |
@owais |
Couple of minor changes/additions suggested. Please also remove the commented out code. |
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.
Looks good 👍 Minor changes requested
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py
Outdated
Show resolved
Hide resolved
…lemetry/instrumentation/flask/__init__.py Co-authored-by: Diego Hurtado <[email protected]>
Co-authored-by: Diego Hurtado <[email protected]>
This reverts commit cbc0995.
…/opentelemetry-python-contrib into flask_hooks" This reverts commit dafb554, reversing changes made to 422d67d.
…sulistio/opentelemetry-python-contrib into flask_hooks"" This reverts commit e91fb0b.
This reverts commit d25c675.
This reverts commit 34e7013.
This reverts commit 34e7013.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
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
Tested using
tox -e test-instrumentation-flask
, wrote two new testsDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.