-
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
Changes from all commits
eb972ea
1b90a0d
00e72c8
171e544
c66af2f
652782f
6767d0b
a836cc9
9fc87b4
6a98f52
f8e2f4d
f4258c7
ab99504
a2df1ac
8fa2090
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
OpenTelemetry requests integration | ||
================================== | ||
|
||
This library allows tracing HTTP requests made by the popular `requests <(https://2.python-requests.org//en/latest/>` library. | ||
|
||
Usage | ||
----- | ||
|
||
.. code-block:: python | ||
|
||
import requests | ||
import opentelemetry.ext.http_requests | ||
from opentelemetry.trace import tracer | ||
|
||
opentelemetry.ext.http_requests.enable(tracer()) | ||
response = requests.get(url='https://www.example.org/') | ||
|
||
Limitations | ||
----------- | ||
|
||
Note that calls that do not use the higher-level APIs but use | ||
:code:`requests.sessions.Session.send` (or an alias thereof) directly, are | ||
currently not traced. If you find any other way to trigger an untraced HTTP | ||
request, please report it via a GitHub issue with :code:`[requests: untraced | ||
API]` in the title. | ||
|
||
References | ||
---------- | ||
|
||
* `OpenTelemetry Project <https://opentelemetry.io/>`_ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
# | ||
[metadata] | ||
name = opentelemetry-ext-http-requests | ||
description = OpenTelemetry requests integration | ||
long_description = file: README.rst | ||
long_description_content_type = text/x-rst | ||
author = OpenTelemetry Authors | ||
author_email = [email protected] | ||
url = https://github.com/open-telemetry/opentelemetry-python/ext/opentelemetry-ext-http-requests | ||
platforms = any | ||
license = Apache-2.0 | ||
classifiers = | ||
Development Status :: 3 - Alpha | ||
Intended Audience :: Developers | ||
License :: OSI Approved :: Apache Software License | ||
Programming Language :: Python | ||
Programming Language :: Python :: 3 | ||
Programming Language :: Python :: 3.4 | ||
Programming Language :: Python :: 3.5 | ||
Programming Language :: Python :: 3.6 | ||
Programming Language :: Python :: 3.7 | ||
|
||
[options] | ||
python_requires = >=3.4 | ||
package_dir= | ||
=src | ||
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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I think From pep 440, I think this translates to |
||
|
||
[options.packages.find] | ||
where = src |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
import os | ||
import setuptools | ||
|
||
BASE_DIR = os.path.dirname(__file__) | ||
VERSION_FILENAME = os.path.join( | ||
BASE_DIR, | ||
"src", | ||
"opentelemetry", | ||
"ext", | ||
"http_requests", | ||
"version.py", | ||
) | ||
PACKAGE_INFO = {} | ||
with open(VERSION_FILENAME) as f: | ||
exec(f.read(), PACKAGE_INFO) | ||
|
||
setuptools.setup(version=PACKAGE_INFO["__version__"]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
""" | ||
The opentelemetry-ext-requests package allows tracing HTTP requests made by the | ||
popular requests library. | ||
""" | ||
|
||
from urllib.parse import urlparse | ||
import functools | ||
|
||
from requests.sessions import Session | ||
|
||
|
||
# NOTE: Currently we force passing a tracer. But in turn, this forces the user | ||
# to configure a SDK before enabling this integration. In turn, this means that | ||
# 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 commentThe 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. |
||
"""Enables tracing of all requests calls that go through | ||
:code:`requests.session.Session.request` (this includes | ||
:code:`requests.get`, etc.).""" | ||
|
||
# Since | ||
# https://github.com/psf/requests/commit/d72d1162142d1bf8b1b5711c664fbbd674f349d1 | ||
# (v0.7.0, Oct 23, 2011), get, post, etc are implemented via request which | ||
# again, is implemented via Session.request (`Session` was named `session` | ||
# before v1.0.0, Dec 17, 2012, see | ||
# https://github.com/psf/requests/commit/4e5c4a6ab7bb0195dececdd19bb8505b872fe120) | ||
|
||
# Guard against double instrumentation | ||
disable() | ||
|
||
wrapped = Session.request | ||
|
||
@functools.wraps(wrapped) | ||
def instrumented_request(self, method, url, *args, **kwargs): | ||
# TODO: Check if we are in an exporter, cf. OpenCensus | ||
# execution_context.is_exporter() | ||
|
||
# See | ||
# https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md#http-client | ||
try: | ||
parsed_url = urlparse(url) | ||
except ValueError as exc: # Invalid URL | ||
path = "<Unparsable URL: {}>".format(exc) | ||
else: | ||
if parsed_url is None: | ||
path = "<URL parses to None>" | ||
path = parsed_url.path | ||
|
||
with tracer.start_span(path) as span: | ||
span.set_attribute("component", "http") | ||
# TODO: The OTel spec says "SpanKind" MUST be "Client" but that | ||
# seems to be a leftover, as Spans have no explicit field for | ||
# kind. | ||
span.set_attribute("http.method", method.upper()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
span.set_attribute("http.url", url) | ||
|
||
# TODO: Propagate the trace context via headers once we have a way | ||
# to access propagators. | ||
|
||
result = wrapped(self, method, url, *args, **kwargs) # *** PROCEED | ||
|
||
span.set_attribute("http.status_code", result.status_code) | ||
span.set_attribute("http.status_text", result.reason) | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. We will need to capture exception, call |
||
# certain attributes? | ||
|
||
instrumented_request.opentelemetry_ext_requests_applied = True | ||
|
||
Session.request = instrumented_request | ||
|
||
# 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 commentThe reason will be displayed to describe this comment to others. Learn more. Would it work to instrument only There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
# different, then push the current URL, pop it afterwards) | ||
|
||
|
||
def disable(): | ||
"""Disables instrumentation of :code:`requests` through this module. | ||
|
||
Note that this only works if no other module also patches requests.""" | ||
|
||
if getattr(Session.request, "opentelemetry_ext_requests_applied", False): | ||
original = Session.request.__wrapped__ # pylint:disable=no-member | ||
Session.request = original |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
__version__ = "0.1.dev0" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
import unittest | ||
from unittest import mock | ||
import sys | ||
|
||
import requests | ||
import urllib3 | ||
|
||
import opentelemetry.ext.http_requests | ||
from opentelemetry import trace | ||
|
||
|
||
class TestRequestsIntegration(unittest.TestCase): | ||
|
||
# TODO: Copy & paste from test_wsgi_middleware | ||
def setUp(self): | ||
self.span_attrs = {} | ||
self.tracer = trace.tracer() | ||
self.span_context_manager = mock.MagicMock() | ||
self.span = mock.create_autospec( | ||
trace.Span, spec_set=True | ||
) | ||
self.span_context_manager.__enter__.return_value = self.span | ||
|
||
def setspanattr(key, value): | ||
self.assertIsInstance(key, str) | ||
self.span_attrs[key] = value | ||
|
||
self.span.set_attribute = setspanattr | ||
self.start_span_patcher = mock.patch.object( | ||
self.tracer, | ||
"start_span", | ||
autospec=True, | ||
spec_set=True, | ||
return_value=self.span_context_manager | ||
) | ||
self.start_span = self.start_span_patcher.start() | ||
|
||
mocked_response = requests.models.Response() | ||
mocked_response.status_code = 200 | ||
mocked_response.reason = "Roger that!" | ||
self.send_patcher = mock.patch.object( | ||
requests.Session, "send", autospec=True, spec_set=True, | ||
return_value=mocked_response) | ||
self.send = self.send_patcher.start() | ||
|
||
opentelemetry.ext.http_requests.enable(self.tracer) | ||
|
||
def tearDown(self): | ||
opentelemetry.ext.http_requests.disable() | ||
self.send_patcher.stop() | ||
self.start_span_patcher.stop() | ||
|
||
def test_basic(self): | ||
url = "https://www.example.org/foo/bar?x=y#top" | ||
_response = requests.get(url=url) | ||
self.assertEqual(1, len(self.send.call_args_list)) | ||
self.tracer.start_span.assert_called_with("/foo/bar") | ||
self.span_context_manager.__enter__.assert_called_with() | ||
self.span_context_manager.__exit__.assert_called_with(None, None, None) | ||
self.assertEqual(self.span_attrs, { | ||
"component": "http", | ||
"http.method": "GET", | ||
"http.url": url, | ||
"http.status_code": 200, | ||
"http.status_text": "Roger that!" | ||
}) | ||
|
||
def test_invalid_url(self): | ||
url = "http://[::1/nope" | ||
exception_type = requests.exceptions.InvalidURL | ||
if (sys.version_info[:2] < (3, 5) | ||
and tuple(map( | ||
int, urllib3.__version__.split('.')[:2])) < (1, 25)): | ||
exception_type = ValueError | ||
|
||
with self.assertRaises(exception_type): | ||
_response = requests.post(url=url) | ||
self.assertTrue(self.tracer.start_span.call_args[0][0].startswith( | ||
"<Unparsable URL"), msg=self.tracer.start_span.call_args) | ||
self.span_context_manager.__enter__.assert_called_with() | ||
exitspan = self.span_context_manager.__exit__ | ||
self.assertEqual(1, len(exitspan.call_args_list)) | ||
self.assertIs(exception_type, exitspan.call_args[0][0]) | ||
self.assertIsInstance(exitspan.call_args[0][1], exception_type) | ||
self.assertEqual(self.span_attrs, { | ||
"component": "http", | ||
"http.method": "POST", | ||
"http.url": url, | ||
}) |
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.