Skip to content
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

Restoring metrics in requests #1110

Merged
merged 23 commits into from
Jun 25, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
784bb02
Restoring metrics in requests
ashu658 May 31, 2022
feb34d8
Updating CHANGELOG.md
ashu658 May 31, 2022
0d58aaa
Fixing build errors
ashu658 Jun 2, 2022
cc33431
Fixing unit tests
ashu658 Jun 6, 2022
c5224fc
Fixing unit tests
ashu658 Jun 6, 2022
f39393d
Merge branch 'main' into restore-requests-metrics
ashu658 Jun 6, 2022
f497101
Fixing lint errors
ashu658 Jun 6, 2022
4cca547
Using memory_metrics_reader in unit tests
ashu658 Jun 7, 2022
7f843cb
Merge branch 'main' into restore-requests-metrics
ashu658 Jun 14, 2022
b872ada
Changed docstring to remove errors
ashu658 Jun 14, 2022
5c8cacb
Fixing lint errors
ashu658 Jun 14, 2022
68e34ee
Merge branch 'main' into restore-requests-metrics
ocelotl Jun 15, 2022
80bdff2
Resolving comments: Removing url tag and adding new attributes
ashu658 Jun 21, 2022
7db639b
Merge branch 'main' into restore-requests-metrics
ashu658 Jun 21, 2022
89d8f6e
Removing unwanted changes
ashu658 Jun 21, 2022
b273409
Update instrumentation/opentelemetry-instrumentation-requests/tests/t…
ashu658 Jun 22, 2022
52de6c3
Resolving comments:
ashu658 Jun 23, 2022
661843b
Merge branch 'restore-requests-metrics' of https://github.com/ashu658…
ashu658 Jun 23, 2022
8a0e994
Removing extra linebreak from docstring
ashu658 Jun 23, 2022
077f4be
Merge branch 'main' into restore-requests-metrics
ashu658 Jun 23, 2022
7f4a49b
Fixing lint error
ashu658 Jun 23, 2022
32dab15
Adding _support_metrics variable
ashu658 Jun 24, 2022
e57651b
Adding new line at the end of file
ashu658 Jun 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([1109](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1109))
- cleanup type hints for textmap `Getter` and `Setter` classes
([1106](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1106))

### Added
- `opentelemetry-instrumentation-remoulade` Initial release
([#1082](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1082))

- `opentelemetry-instrumentation-requests` Restoring metrics in requests
([1110](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1110)
ashu658 marked this conversation as resolved.
Show resolved Hide resolved
### Added
- Added `opentelemetry-instrumention-confluent-kafka`
([#1111](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1111))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@

import functools
import types
from time import time
from typing import Collection
from urllib.parse import urlparse

from requests.models import Response
from requests.sessions import Session
Expand All @@ -64,6 +66,7 @@
_SUPPRESS_INSTRUMENTATION_KEY,
http_status_to_status_code,
)
ashu658 marked this conversation as resolved.
Show resolved Hide resolved
from opentelemetry.metrics import get_meter
from opentelemetry.propagate import inject
from opentelemetry.semconv.trace import SpanAttributes
from opentelemetry.trace import SpanKind, get_tracer
Expand All @@ -87,7 +90,11 @@
# pylint: disable=unused-argument
# pylint: disable=R0915
def _instrument(
tracer, span_callback=None, name_callback=None, excluded_urls=None
tracer,
span_callback=None,
name_callback=None,
excluded_urls=None,
metric_recorder=None,
ocelotl marked this conversation as resolved.
Show resolved Hide resolved
ashu658 marked this conversation as resolved.
Show resolved Hide resolved
):
"""Enables tracing of all requests calls that go through
:code:`requests.session.Session.request` (this includes
Expand Down Expand Up @@ -143,6 +150,7 @@ def call_wrapped():
request.method, request.url, call_wrapped, get_or_create_headers
)

# pylint: disable-msg=too-many-locals,too-many-branches
def _instrumented_requests_call(
method: str, url: str, call_wrapped, get_or_create_headers
):
Expand All @@ -167,6 +175,18 @@ def _instrumented_requests_call(
SpanAttributes.HTTP_URL: url,
}

metric_labels = {
"http.method": method,
"http.url": url,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this url parametrized or plain raw url? Otherwise, this can lead very to high cardinality.

Copy link
Member Author

@ashu658 ashu658 Jun 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ashu658 ashu658 Jun 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

try:
parsed_url = urlparse(url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@ashu658 ashu658 Jun 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

metric_labels["http.host"] = parsed_url.hostname
metric_labels["http.scheme"] = parsed_url.scheme
except ValueError:
pass

with tracer.start_as_current_span(
span_name, kind=SpanKind.CLIENT, attributes=span_attributes
) as span, set_ip_on_next_http_connection(span):
Expand All @@ -178,6 +198,9 @@ def _instrumented_requests_call(
token = context.attach(
context.set_value(_SUPPRESS_HTTP_INSTRUMENTATION_KEY, True)
)

start_time = time()
ashu658 marked this conversation as resolved.
Show resolved Hide resolved

try:
result = call_wrapped() # *** PROCEED
except Exception as exc: # pylint: disable=W0703
Expand All @@ -194,9 +217,25 @@ def _instrumented_requests_call(
span.set_status(
Status(http_status_to_status_code(result.status_code))
)

metric_labels["http.status_code"] = result.status_code

if result.raw is not None:
version = getattr(result.raw, "version", None)
if version:
metric_labels["http.flavor"] = (
"1.1" if version == 11 else "1.0"
)

if span_callback is not None:
span_callback(span, result)

elapsed_time = time() - start_time
ashu658 marked this conversation as resolved.
Show resolved Hide resolved

metric_recorder.record(
elapsed_time * 1000, attributes=metric_labels
)

if exception is not None:
raise exception.with_traceback(exception.__traceback__)

Expand Down Expand Up @@ -261,13 +300,25 @@ def _instrument(self, **kwargs):
tracer_provider = kwargs.get("tracer_provider")
tracer = get_tracer(__name__, __version__, tracer_provider)
excluded_urls = kwargs.get("excluded_urls")
meter_provider = kwargs.get("meter_provider")
meter = get_meter(
__name__,
__version__,
meter_provider,
)
metric_recorder = meter.create_histogram(
ashu658 marked this conversation as resolved.
Show resolved Hide resolved
name="http.client.duration",
unit="ms",
description="measures the duration of the outbound HTTP request",
)
_instrument(
tracer,
span_callback=kwargs.get("span_callback"),
name_callback=kwargs.get("name_callback"),
excluded_urls=_excluded_urls_from_env
if excluded_urls is None
else parse_excluded_urls(excluded_urls),
metric_recorder=metric_recorder,
)

def _uninstrument(self, **kwargs):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
from opentelemetry.instrumentation.utils import _SUPPRESS_INSTRUMENTATION_KEY
from opentelemetry.propagate import get_global_textmap, set_global_textmap
from opentelemetry.sdk import resources
from opentelemetry.sdk.metrics import MeterProvider
from opentelemetry.sdk.metrics.export import InMemoryMetricReader
from opentelemetry.semconv.trace import SpanAttributes
from opentelemetry.test.mock_textmap import MockTextMapPropagator
from opentelemetry.test.test_base import TestBase
Expand Down Expand Up @@ -472,3 +474,49 @@ def perform_request(url: str, session: requests.Session = None):
request = requests.Request("GET", url)
prepared_request = session.prepare_request(request)
return session.send(prepared_request)


class TestRequestsIntergrationMetric(TestBase):
URL = "http://httpbin.org/status/200"

def setUp(self):
super().setUp()
self.reader = InMemoryMetricReader()
self.meter_provider = MeterProvider(metric_readers=[self.reader])
ashu658 marked this conversation as resolved.
Show resolved Hide resolved
RequestsInstrumentor().instrument(meter_provider=self.meter_provider)

httpretty.enable()
httpretty.register_uri(httpretty.GET, self.URL, body="Hello!")

def tearDown(self):
super().tearDown()
RequestsInstrumentor().uninstrument()
httpretty.disable()

@staticmethod
def perform_request(url: str) -> requests.Response:
return requests.get(url)

def test_basic_http_success(self):
self.perform_request(self.URL)

expected_attributes = {
"http.status_code": 200,
"http.url": self.URL,
"http.host": "httpbin.org",
"http.method": "GET",
"http.flavor": "1.1",
"http.scheme": "http",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing a few? net.*, http.target?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

for (
resource_metrics
) in self.reader.get_metrics_data().resource_metrics:
for scope_metrics in resource_metrics.scope_metrics:
for metric in scope_metrics.metrics:
for data_point in metric.data.data_points:
print(dict(data_point.attributes))
self.assertDictEqual(
expected_attributes, dict(data_point.attributes)
)
self.assertEqual(data_point.count, 1)