Add optional http.client.response.body.size span attribute & metric to Aiohttp-client instrumentation.#4572
Add optional http.client.response.body.size span attribute & metric to Aiohttp-client instrumentation.#4572weizhikuan wants to merge 5 commits into
Conversation
|
As per the semconv spec v1.41.0, the span attribute and metric are both optional. So I added both of them and only enabled when semconv stability contains http and the env var |
|
Thanks for the PR! Just a heads-up: we no longer update Please add the appropriate changelog fragment for this change instead of editing |
|
Thank you for this @weizhikuan . The way to opt into this makes sense. Please do a quick |
Gotcha, will add the fragment. |
| **************************** | ||
| To capture the ``http.response.body.size`` span attribute and record the | ||
| ``http.client.response.body.size`` metric histogram, set the environment variable | ||
| ``OTEL_PYTHON_INSTRUMENTATION_HTTP_RESPONSE_BODY_SIZE`` to ``"true"``. |
There was a problem hiding this comment.
Personally, I don't think that having an opt-in is really needed here. From my understanding, when the Spec states that a metric or attribute is "optional", it usually indicates that library authors can optionally add it - not necessarily that users need the ability to enable/disable it. Typically, the spec will explicitly state if behavior should be opt-in only.
herin049
moved this from Ready for review
to Reviewed PRs that need fixes
in Python PR digest
| ): | ||
| return | ||
|
|
||
| trace_config_ctx.start_time = default_timer() |
There was a problem hiding this comment.
We should probably add the following, just to ensure that the response body size doesn't accidentally leak into another request (e.g. if follow-up requests don't end up setting the attribute)
| trace_config_ctx.start_time = default_timer() | |
| trace_config_ctx.response_body_size = None |
5 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #3623
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added unit tests.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.