Aiohttp server instrumentation#942
Conversation
|
| ): | ||
| return await handler(request) | ||
|
|
||
| token = context.attach(extract(request, getter=getter)) |
There was a problem hiding this comment.
context.attach here is incorrect. The request being handled is from an external service, and the context contained within that request is a representation of the state of the external service. It doesn't make sense to set the external service's context as the current context for the service running this middleware.
The context should still be extracted from the request and supplied to tracer.start_as_current_span, but the attach and detach can be removed.
| span_name, | ||
| kind=trace.SpanKind.SERVER, | ||
| ) as span: | ||
| if span.is_recording(): |
There was a problem hiding this comment.
I think this if statement should be removed. In the case that this is a non-recording span, set_attribute is a no-op. Additionally, this middleware should always call the request handler (line 158).
| for key, value in attributes.items(): | ||
| span.set_attribute(key, value) |
There was a problem hiding this comment.
Replace with span.setattributes(attributes).
| try: | ||
| resp = await handler(request) | ||
| set_status_code(span, resp.status) | ||
| finally: | ||
| context.detach(token) |
There was a problem hiding this comment.
| try: | |
| resp = await handler(request) | |
| set_status_code(span, resp.status) | |
| finally: | |
| context.detach(token) | |
| resp = await handler(request) | |
| set_status_code(span, resp.status) |
| if ( | ||
| context.get_value("suppress_instrumentation") | ||
| or context.get_value(_SUPPRESS_HTTP_INSTRUMENTATION_KEY) | ||
| or _excluded_urls.url_disabled(request.url) |
There was a problem hiding this comment.
request.url is not a str. This should be request.url.path.
|
@dmanchon I'm interested in testing and maybe helping to finish this PR? How can I test this PR after it's checked out? I have an aiohttp based app to test with. Also what do you know that needs finishing? The reviewer comments look all pretty straightforward, so after that what else is left? |
| install_requires = | ||
| opentelemetry-api ~= 1.3 | ||
| opentelemetry-semantic-conventions == 0.28b1 | ||
| opentelemetry-instrumentation == 0.28b1 | ||
| opentelemetry-util-http == 0.28b1 |
There was a problem hiding this comment.
These requirements should be updated to the latest version
|
|
||
| [options.extras_require] | ||
| test = | ||
| opentelemetry-test-utils == 0.28b1 |
There was a problem hiding this comment.
Update to latest version
|
Sorry @kennytrytek-wf @bmbouter for late response, i completely forgot about this. We currently have a vendored version of this in our codebase and given the lack of response here and in the official slack channel i undertood that there were not interest. |
Both server and client in one package is probably the best option by virtue of being simple + not real upsides of separating it. It's ultimately not that much extra code. |
|
@dmanchon correct me if I'm wrong, but I didn't see any code for metrics. Is that right? Would be interesting to have it in this PR? |
|
@decko I would just open a new PR from your own branch which has these commits |
| ) as span: | ||
| attributes = collect_request_attributes(request) | ||
| attributes.update(additional_attributes) | ||
| span.setattributes(attributes) |
There was a problem hiding this comment.
little misspelling
| span.setattributes(attributes) | |
| span.set_attributes(attributes) |
|
Duplicate of #1800 |
7 participants
Description
Instrument aiohttp server. Right now there is only instrumentation for the client.
Type of change
Please delete options that are not relevant.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.