-
Notifications
You must be signed in to change notification settings - Fork 595
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
ext/requests: Add instrumentor #597
ext/requests: Add instrumentor #597
Conversation
Implement Instrumentor interface to allow user enabling instrumentation in the library and to work with autoinstrumentation.
Set span status from http status 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.
This is looking great. I tested it with opentelemetry-auto-instrumentation
and got some traces with the following code
from opentelemetry import trace
from opentelemetry.sdk.trace.export import (
ConsoleSpanExporter,
SimpleExportSpanProcessor,
)
import requests
trace.get_tracer_provider().add_span_processor(
SimpleExportSpanProcessor(ConsoleSpanExporter())
)
url = f"https://wikipedia.org"
try:
res = requests.get(url)
print(f"Request to {url}, got {len(res.content)} bytes")
except Exception as e:
print(f"Request to {url} failed {e}")
@@ -28,15 +28,18 @@ | |||
|
|||
# The preferred tracer implementation must be set, as the opentelemetry-api | |||
# defines the interface with a no-op implementation. | |||
# It must be done before instrumenting any library. |
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.
Will this change once the instrumentor interface supports configuration?
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.
In this particular example all the calls to instrument()
omit the tracer_provider
parameter, it means the current configured is used. If there is not any configured, and OPENTELEMETRY_PYTHON_TRACER_PROVIDER
is not set, a default no-op will be used.
I think the common approach we should suggest in the examples is.
- trace.set_tracer_provider(TracerProvider()) (We don't want to mess up with env variables in the examples)
- instrument the libraries
- configure the span processor and exporters
- use of the instrumented framework.
opentelemetry.ext.http_requests.enable(TracerProvider()) | ||
response = requests.get(url='https://www.example.org/') | ||
# You can optionally pass a custom TracerProvider to RequestInstrumentor.instrument() | ||
opentelemetry.ext.http_requests.RequestInstrumentor.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.
should this example show how to pass a TracerProvider?
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 think so. Passing a TracerProvider is an advanced use case, we don't expect it to be common.
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.
how about the example with the kwarg as none: instrument(tracer_prover=None)
. If I saw that comment, I would have no idea what the argument name is for passing my own TracerProvider.
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 this documentation should be as simple as possible and the documentation about the parameters of such function should be on the generated documentation, that we don't know how to do right now: https://github.com/open-telemetry/opentelemetry-python/issues/617.
_tracer = None | ||
|
||
|
||
def _http_status_to_canonical_code(code: int, allow_redirect: bool = True): |
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.
nice!
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.
should we follow up and move this somewhere up the packages? this is duplicated in 4-5 places now.
I hesitate to suggest the api package, which is the only common dependency. Or maybe an opentelemetry-instrumentor-utils?
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 we should move that to a new package in the future, I'd like to avoid doing that on this PR as I want this to be in asap. By the way, I opened an issue to initiate the discussion about that #610, I think an utils package for instrumentors is the best idea.
@@ -103,12 +110,12 @@ def instrumented_request(self, method, url, *args, **kwargs): | |||
|
|||
span.set_attribute("http.status_code", result.status_code) | |||
span.set_attribute("http.status_text", result.reason) | |||
span.set_status( | |||
Status(_http_status_to_canonical_code(result.status_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.
did you want to pass an argument for allow_redirect
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.
I took the code from
opentelemetry-python/ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py
Line 90 in 7cb57c7
def http_status_to_canonical_code(code: int, allow_redirect: bool = True): |
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.
Looking at that code, it only applies to the 399 status code, which is unassigned. @Oberon00 any insight into the rationalle for the parameter, and the 399 status code? Looks like you were one of the authors of that commit.
@@ -29,23 +30,25 @@ class TestRequestsIntegration(TestBase): | |||
|
|||
def setUp(self): | |||
super().setUp() | |||
opentelemetry.ext.http_requests.enable(self.tracer_provider) | |||
http_requests.RequestsInstrumentor().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.
should this be passing in the tracer_provider?
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.
TestBase is doing trace.set_tracer_provider(cls.tracer_provider)
, so it's the same.
@codeboten just as a reference, the example you provided requires |
opentelemetry.ext.http_requests.enable(TracerProvider()) | ||
response = requests.get(url='https://www.example.org/') | ||
# You can optionally pass a custom TracerProvider to RequestInstrumentor.instrument() | ||
opentelemetry.ext.http_requests.RequestInstrumentor.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.
how about the example with the kwarg as none: instrument(tracer_prover=None)
. If I saw that comment, I would have no idea what the argument name is for passing my own TracerProvider.
@@ -103,12 +110,12 @@ def instrumented_request(self, method, url, *args, **kwargs): | |||
|
|||
span.set_attribute("http.status_code", result.status_code) | |||
span.set_attribute("http.status_text", result.reason) | |||
span.set_status( | |||
Status(_http_status_to_canonical_code(result.status_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.
Looking at that code, it only applies to the 399 status code, which is unassigned. @Oberon00 any insight into the rationalle for the parameter, and the 399 status code? Looks like you were one of the authors of that commit.
_tracer = None | ||
|
||
|
||
def _http_status_to_canonical_code(code: int, allow_redirect: bool = True): |
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.
should we follow up and move this somewhere up the packages? this is duplicated in 4-5 places now.
I hesitate to suggest the api package, which is the only common dependency. Or maybe an opentelemetry-instrumentor-utils?
docs/examples/opentelemetry-example-app/src/opentelemetry_example_app/flask_example.py
Show resolved
Hide resolved
ext/opentelemetry-ext-http-requests/src/opentelemetry/ext/http_requests/__init__.py
Show resolved
Hide resolved
ext/opentelemetry-ext-http-requests/src/opentelemetry/ext/http_requests/__init__.py
Show resolved
Hide resolved
ext/opentelemetry-ext-http-requests/src/opentelemetry/ext/http_requests/__init__.py
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! Just a few non-blocking questions/comments. Good work!
* chore(plugin-https): sync https tests with http * chore: use Http instead typeof http * chore: review finding, improve https detection * chore: fix node 8 * chore: fix path to test files
Implement the
BaseInstrumentor
interface to make this library compatible with the opentelemetry-auto-instr command.There is an issue about getting the span when the global tracer provider hasn't been configured, this should be changed in the future once we extend the opentelemetry-auto-instr command to also configure the SDK.