aiohttp enhancements #259
aiohttp enhancements #259
Conversation
- only mark 400+ statuses as errors (don't mark 2xx) - attempt fixing unittests
spent waaaay too long trying to get the lambda moto test to work with aiobotocore
| PARENT_TRACE_HEADER_ID = 'x-ddtrace-parent_trace_id' | ||
| PARENT_SPAN_HEADER_ID = 'x-ddtrace-parent_span_id' | ||
|
|
||
| SPAN_MIN_ERROR = 400 |
There was a problem hiding this comment.
Thanks for this PR! We got many requests for both functionalities (the automatic distributed tracing and choosing the minimum status code that is considered an error). Actually we're still debating internally for both features, because this change is going to impact all other integrations (and even supported languages/frameworks).
Probably I have to keep this PR on-hold for a bit more until we decided how to generally handle that. Anyway thanks for this contribution! We'll give priority to all these specs so that your PR can be merged as soon as possible!
# Conflicts: # ddtrace/contrib/aiobotocore/patch.py # ddtrace/monkey.py # docs/index.rst # tests/contrib/aiobotocore/test.py # tox.ini
|
btw, this test failure doesn't seem to be my fault? |
|
@thehesiod inconsistent test maybe, we should update that because it fails some times. Will relaunch our CI! |
| @@ -77,11 +101,17 @@ def on_prepare(request, response): | |||
| request_span.finish() | |||
|
|
|||
|
|
|||
| def trace_app(app, tracer, service='aiohttp-web'): | |||
| def trace_app(app, tracer, service='aiohttp-web', enable_distributed=False): | |||
There was a problem hiding this comment.
@palazzem should we remove the tracer from here too? Would not be backwards compatible though
There was a problem hiding this comment.
in that case no, since it configures an application object instead of patching it; anyway, the idea is to remove that too but in a later change
| if e.status_code >= _SPAN_MIN_ERROR: | ||
| request_span.set_traceback() | ||
| raise | ||
| except BaseException: |
There was a problem hiding this comment.
I think Exception is enough, because we're not sending data when SystemExit (or similar) occurs.
| PARENT_TRACE_HEADER_ID = 'x-ddtrace-parent_trace_id' | ||
| PARENT_SPAN_HEADER_ID = 'x-ddtrace-parent_span_id' | ||
|
|
||
| _SPAN_MIN_ERROR = 400 |
There was a problem hiding this comment.
at the moment I think we should keep this out; while I know that it's really helpful for you, I think that this PR should not include the change so we can easily proceed with the merge without waiting the specification for "what should be considered an error by default" and "how users can change that behavior in general".
Is it fine if you remove the _SPAN_MIN_ERROR from this PR, moving this in your personal fork (or another PR if you prefer).
|
|
||
| @asyncio.coroutine | ||
| def trace_middleware(app, handler): | ||
| def trace_middleware(app, handler, enable_distributed=False): |
There was a problem hiding this comment.
since we're already using configs here, what do you think if we put enable_distributed as a config object instead of having it as a kwarg here? so you can do:
distributed_tracing = app[CONFIG_KEY]['enable_distributed_tracing']and users can set that in the application config
| @@ -77,11 +101,17 @@ def on_prepare(request, response): | |||
| request_span.finish() | |||
|
|
|||
|
|
|||
| def trace_app(app, tracer, service='aiohttp-web'): | |||
| def trace_app(app, tracer, service='aiohttp-web', enable_distributed=False): | |||
There was a problem hiding this comment.
in that case no, since it configures an application object instead of patching it; anyway, the idea is to remove that too but in a later change
| @@ -105,5 +135,7 @@ def trace_app(app, tracer, service='aiohttp-web'): | |||
|
|
|||
| # add the async tracer middleware as a first middleware | |||
| # and be sure that the on_prepare signal is the last one | |||
| app.middlewares.insert(0, trace_middleware) | |||
| app.middlewares.insert(0, functools.partial( | |||
| trace_middleware, enable_distributed=enable_distributed)) | |||
There was a problem hiding this comment.
if the previous comment makes sense, we can revert that change
| @@ -9,6 +9,9 @@ | |||
| REQUEST_CONTEXT_KEY = 'datadog_context' | |||
| REQUEST_SPAN_KEY = '__datadog_request_span' | |||
|
|
|||
| PARENT_TRACE_HEADER_ID = 'x-datadog-trace-id' | |||
| PARENT_SPAN_HEADER_ID = 'x-datadog-parent-id' | |||
There was a problem hiding this comment.
@thehesiod I've changed the headers so that they're compliant with other integrations (like our Go client)
There was a problem hiding this comment.
with this latest change it's good to go! @thehesiod unfortunately I had to remove your MIN_ERROR patch, since it requires more time to discuss about it, while the rest is OK since it's disabled by default.
You may re-add the patch in your personal fork and, hopefully, have it merge in the near future.
pulled out aiohttp enhancements from #248