Restoring metrics in requests #1110
Conversation
|
|
ashu658
changed the title
ashu658
force-pushed
the
restore-requests-metrics
branch
from
d94f67e to
784bb02
Compare
ashu658
changed the title
instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py
Outdated
Show resolved
Hide resolved
| @@ -167,6 +175,18 @@ def _instrumented_requests_call( | |||
| SpanAttributes.HTTP_URL: url, | |||
| } | |||
|
|
|||
| metric_labels = { | |||
| "http.method": method, | |||
| "http.url": url, | |||
There was a problem hiding this comment.
Is this url parametrized or plain raw url? Otherwise, this can lead very to high cardinality.
There was a problem hiding this comment.
This is the same url we add in span attributes. I think its not parameterised. (i.e. when added in metric it would look something like this http.url: STRING(https://www.example.com/123/?key1=value1)
I cannot think of a way to find encoded parameters from url to remove them. (e.g. in the above example remove 123 from path).
Open for suggestions :)
There was a problem hiding this comment.
That's not a problem with tracing but here each url creates a new time series which should be avoided. If there is no way to get the parametrized url I would suggest remove this tag entirely.
There was a problem hiding this comment.
Removed url tag. But there could be 2 different urls in a single time series (as there might be no differentiating tag). I am not sure if this is okay. Any suggestions?
...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| try: | ||
| parsed_url = urlparse(url) |
There was a problem hiding this comment.
I imagine that the call to urlparse is what can cause the ValueError exception to be raised. If that happens, does it make sense to continue the instrumentation with a metrics_labels dictionary that will not have "http.host" and "http.scheme" keys?
There was a problem hiding this comment.
In traces we add the url even if the url is invalid (e.g. url = random123 is also added as span attribute). So, I added it here for consistency.
I think its okay to go without http.host and http.scheme keys as http.url attribute alone is one of the recommended set of attributes for client metrics (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives)
...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
Show resolved
Hide resolved
| "http.host": "httpbin.org", | ||
| "http.method": "GET", | ||
| "http.flavor": "1.1", | ||
| "http.scheme": "http", |
There was a problem hiding this comment.
Are we missing a few? net.*, http.target?
There was a problem hiding this comment.
Added net.* attributes.
I have not added http.target as the target is not parameterised and adding raw target can lead to high cardinality.
Its similar for http.url as discussed here
instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
Outdated
Show resolved
Hide resolved
…est_requests_integration.py Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
1) Using default_timer 2) Refactoring varibale names 3) Removing metric usage docstring
…/opentelemetry-python-contrib into restore-requests-metrics
Description
Restoring production of metrics for requests library.
Fixes #1039
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested by sending metrics to collector and added unit test.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.