Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[FLASK] added request and response hook #416
Changes from 9 commits
a0cdcc2
53ddfcd
c0ab6a5
ddb3077
ea965d1
74bc40c
8a4f2a3
4721cdf
79d04fe
f975067
1605884
7bffdba
84bbec1
5eaeaf2
42e9372
dc63d37
738065c
a520dfa
a308044
2757171
327905e
6dd503c
2f2611e
e941831
34e7013
5534ed1
b867ead
ddc10d7
cbc0995
d4eab75
422d67d
dafb554
d25c675
e91fb0b
aa37da5
7b017ec
add1367
a8f353e
1aafe2b
e95313f
ff10d2b
52cfe53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.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 callFlaskInstrumentor().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.