-
Notifications
You must be signed in to change notification settings - Fork 624
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
Requests integration #94
Requests integration #94
Conversation
2f97089
to
45ecfa1
Compare
# TODO: The OTel spec says "SpanKind" MUST be "Client" but that | ||
# seems to be a leftover, as Spans have no explicit "Client" | ||
# field. | ||
span.set_attribute("http.method", method.upper()) |
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.
Curious, when do we prefer double quote vs. single quote for literal strings?
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.
Ah, true. I have been inconsistent here. Normally I just use single quotes for everything but I read up on Black and it seems to format everything with double quotes.
|
||
return result | ||
|
||
# TODO: How to handle exceptions? Should we create events for them? Set |
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.
We will need to capture exception, call Span.SetStatus
, and rethrow if we got anything.
I guess we don't need to create event for exception as the user might not want it (e.g. the user might decide to capture exception at higher level and attach event). Having a TODO
is good to go for now, this can be a separate PR.
import opentelemetry.ext.http_requests | ||
from opentelemetry.trace import tracer | ||
|
||
enable_requests_integration.enable(tracer()) |
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.
Where is enable_requests_integration
?
|
||
# TODO: We should also instrument requests.sessions.Session.send | ||
# but to avoid doubled spans, we would need some context-local | ||
# state (i.e., only create a Span if the current context's URL is |
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.
Would it work to instrument only requests.sessions.Session.send
?
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.
Then we would see each redirect separately but we wouldn't have an overarching span for the complete logical request including redirects.
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 know we have several TODOs, but this is good enough to move forward :)
ext/opentelemetry-ext-http-requests/tests/test_requests_integration.py
Outdated
Show resolved
Hide resolved
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.
LGTM, we can leave the TODOs for another PR.
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.
Agree with @reyang , this seems like a good start for this integration and the limitations / TODOs are clearly spelled out! 🎉
I will wait for #95 to be merged because I know that I at least used |
# limitations under the License. | ||
# | ||
[metadata] | ||
name = opentelemetry-ext-http-requests |
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.
why "http-requests"? It feels to me like calling this "opentelemetry-ext-requests" might be clearer.
For example, I imagine flask integration being along the lines of "opentelemetry-ext-flask" rather than "opentelemetry-ext-httpserver-requests".
Trying to put how I feel into words, I think one who is looking for an integration will find it via the package name alone, and the additional categorization is unneeded.
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.
@toumorokoshi I kind of agree with you, this is what I did in OpenCensus https://github.com/census-instrumentation/opencensus-python/tree/master/contrib.
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 agree but it seems that pylint gets confused when naming it requests
. Please look at the commit history of this PR.
# if the SDK/tracer is already using `requests` they may, in theory, bypass our | ||
# instrumentation when using `import from`, etc. (currently we only instrument | ||
# a instance method so the probability for that is very low). | ||
def enable(tracer): |
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.
could this be refactored to allow one to invoke injecting the tracer into a single requests Session?
I think the global patch should be a provided method as well, but I think it's important to allow consumers to configure multiple http clients. A global patch doesn't provide that flexibility.
packages=find: | ||
install_requires = | ||
opentelemetry-api >= 0.1.dev0 | ||
requests ~= 2.0 |
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 may cause issues if a consumer wants to use requests greater than 2.0. I would argue we should err on the side of minimal version range restrictions to reduce friction as the requests projects revs their version for whatever reason.
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 mean, we should allow requests3? From reading its homepage it seems to be significantly different though.
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.
Perhaps >=2.0,<3.0 ?
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 requests ~= 2.0
is equivalent to >=2.0,<3.0
.
From pep 440, I think this translates to >=2.0,==2.*
See previous CI failure for pylint issue: ************* Module ext/opentelemetry-ext-requests/src/__init__.py ext/opentelemetry-ext-requests/src/__init__.py:1:0: F0001: No module named ext/opentelemetry-ext-requests/src/__init__.py (fatal) It seems that pylint gets confused when there is more than one "requests" module??
Co-Authored-By: Allan Feldman <[email protected]>
5b56ec3
to
6a98f52
Compare
I'm merging this now, but if someone finds a way to change the name from |
Implements #79 |
This (draft) PR adds an integration for the requests libraray (see README.md).
For some reason, pylint insists on anI seem to have fixed this by renaming the module.__init__.py
in the module root for requests, but not for wsgi. I'm again clueless as to why. 😅More tests will follow.✔️