-
Notifications
You must be signed in to change notification settings - Fork 599
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
Add metric instrumentation for urllib #1553
Changes from 2 commits
89df093
ad9f327
fc8a762
06dadb7
749994b
a961c34
2568f33
5db16c9
53e6b02
c0864ec
15d3231
ea4130a
0d28401
9a74af3
1e9902a
db9400a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,9 +64,9 @@ def response_hook(span, request_obj, response) | |
import types | ||
import typing | ||
|
||
# from urllib import response | ||
from http import client | ||
from typing import Collection | ||
from timeit import default_timer | ||
from typing import Collection, Dict | ||
from urllib.request import ( # pylint: disable=no-name-in-module,import-error | ||
OpenerDirector, | ||
Request, | ||
|
@@ -83,6 +83,8 @@ def response_hook(span, request_obj, response) | |
_SUPPRESS_INSTRUMENTATION_KEY, | ||
http_status_to_status_code, | ||
) | ||
from opentelemetry.metrics import Histogram, get_meter | ||
from opentelemetry.semconv.metrics import MetricInstruments | ||
from opentelemetry.propagate import inject | ||
from opentelemetry.semconv.trace import SpanAttributes | ||
from opentelemetry.trace import Span, SpanKind, get_tracer | ||
|
@@ -114,8 +116,15 @@ def _instrument(self, **kwargs): | |
""" | ||
tracer_provider = kwargs.get("tracer_provider") | ||
tracer = get_tracer(__name__, __version__, tracer_provider) | ||
|
||
meter_provider = kwargs.get("meter_provider") | ||
meter = get_meter(__name__, __version__, meter_provider) | ||
|
||
histograms = _create_client_histograms(meter) | ||
|
||
_instrument( | ||
tracer, | ||
histograms, | ||
request_hook=kwargs.get("request_hook"), | ||
response_hook=kwargs.get("response_hook"), | ||
) | ||
|
@@ -132,6 +141,7 @@ def uninstrument_opener( | |
|
||
def _instrument( | ||
tracer, | ||
histograms: Dict[str, Histogram], | ||
request_hook: _RequestHookT = None, | ||
response_hook: _ResponseHookT = None, | ||
): | ||
|
@@ -192,11 +202,13 @@ def _instrumented_open_call( | |
context.set_value(_SUPPRESS_HTTP_INSTRUMENTATION_KEY, True) | ||
) | ||
try: | ||
start_time = default_timer() | ||
result = call_wrapped() # *** PROCEED | ||
except Exception as exc: # pylint: disable=W0703 | ||
exception = exc | ||
result = getattr(exc, "file", None) | ||
finally: | ||
elapsed_time = round((default_timer() - start_time) * 1000) | ||
context.detach(token) | ||
|
||
if result is not None: | ||
|
@@ -214,6 +226,10 @@ def _instrumented_open_call( | |
SpanAttributes.HTTP_FLAVOR | ||
] = f"{ver_[:1]}.{ver_[:-1]}" | ||
|
||
_record_histograms( | ||
histograms, labels, request, result, elapsed_time | ||
) | ||
|
||
if callable(response_hook): | ||
response_hook(span, request, result) | ||
|
||
|
@@ -248,3 +264,44 @@ def _uninstrument_from(instr_root, restore_as_bound_func=False): | |
if restore_as_bound_func: | ||
original = types.MethodType(original, instr_root) | ||
setattr(instr_root, instr_func_name, original) | ||
|
||
|
||
def _create_client_histograms(meter) -> Dict[str, Histogram]: | ||
histograms = { | ||
MetricInstruments.HTTP_CLIENT_DURATION: meter.create_histogram( | ||
name=MetricInstruments.HTTP_CLIENT_DURATION, | ||
unit="ms", | ||
description="measures the duration outbound HTTP requests", | ||
), | ||
MetricInstruments.HTTP_CLIENT_REQUEST_SIZE: meter.create_histogram( | ||
name=MetricInstruments.HTTP_CLIENT_REQUEST_SIZE, | ||
unit="By", | ||
description="measures the size of HTTP request messages (compressed)", | ||
), | ||
MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE: meter.create_histogram( | ||
name=MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE, | ||
unit="By", | ||
description="measures the size of HTTP response messages (compressed)", | ||
), | ||
} | ||
|
||
return histograms | ||
|
||
|
||
def _record_histograms( | ||
histograms, metric_attributes, request, response, elapsed_time | ||
): | ||
histograms[MetricInstruments.HTTP_CLIENT_DURATION].record( | ||
elapsed_time, attributes=metric_attributes | ||
) | ||
|
||
data = getattr(request, "data", None) | ||
request_size = 0 if data is None else len(data) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this be a compressed size? Does data always return an iterable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the comment, I checked this issue while I wrote the code and the difference is the urllib api works only with bytes, and urllib3 works with bytes and IO(like BytesIO) |
||
histograms[MetricInstruments.HTTP_CLIENT_REQUEST_SIZE].record( | ||
request_size, attributes=metric_attributes | ||
) | ||
|
||
response_size = int(response.headers.get("Content-Length", 0)) | ||
histograms[MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE].record( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question about the compression as the instrument description make it explicit |
||
response_size, attributes=metric_attributes | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,3 +14,5 @@ | |
|
||
|
||
_instruments = tuple() | ||
|
||
_supports_metrics = True |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,248 @@ | ||
# Copyright The OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
|
||
import urllib | ||
from urllib import request | ||
from urllib.parse import urlencode | ||
from timeit import default_timer | ||
from typing import Optional, Union | ||
|
||
import httpretty | ||
from opentelemetry.semconv.metrics import MetricInstruments | ||
|
||
from opentelemetry.instrumentation.urllib import ( # pylint: disable=no-name-in-module,import-error | ||
URLLibInstrumentor, | ||
) | ||
from opentelemetry.sdk.metrics._internal.point import Metric | ||
from opentelemetry.sdk.metrics.export import ( | ||
HistogramDataPoint, | ||
NumberDataPoint, | ||
) | ||
from opentelemetry.test.test_base import TestBase | ||
|
||
|
||
class TestRequestsIntegration(TestBase): | ||
URL = "http://httpbin.org/status/200" | ||
URL_POST = "http://httpbin.org/post" | ||
|
||
def setUp(self): | ||
super().setUp() | ||
URLLibInstrumentor().instrument() | ||
httpretty.enable() | ||
httpretty.register_uri(httpretty.GET, self.URL, body=b"Hello!") | ||
httpretty.register_uri( | ||
httpretty.POST, self.URL_POST, body=b"Hello World!" | ||
) | ||
|
||
def tearDown(self): | ||
super().tearDown() | ||
URLLibInstrumentor().uninstrument() | ||
httpretty.disable() | ||
|
||
def get_sorted_metrics(self): | ||
resource_metrics = ( | ||
self.memory_metrics_reader.get_metrics_data().resource_metrics | ||
) | ||
|
||
all_metrics = [] | ||
for metrics in resource_metrics: | ||
for scope_metrics in metrics.scope_metrics: | ||
all_metrics.extend(scope_metrics.metrics) | ||
|
||
return self.sorted_metrics(all_metrics) | ||
|
||
@staticmethod | ||
def sorted_metrics(metrics): | ||
""" | ||
Sorts metrics by metric name. | ||
""" | ||
return sorted( | ||
metrics, | ||
key=lambda m: m.name, | ||
) | ||
|
||
def assert_metric_expected( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used utils function for metrics assert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have some reservations about the PR on the main repo. I will leave a review there about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! |
||
self, | ||
metric: Metric, | ||
expected_value: Union[int, float], | ||
expected_attributes: dict, | ||
est_delta: Optional[float] = None, | ||
): | ||
data_point = next(iter(metric.data.data_points)) | ||
|
||
if isinstance(data_point, HistogramDataPoint): | ||
self.assertEqual( | ||
data_point.count, | ||
1, | ||
) | ||
if est_delta is None: | ||
self.assertEqual( | ||
data_point.sum, | ||
expected_value, | ||
) | ||
else: | ||
self.assertAlmostEqual( | ||
data_point.sum, | ||
expected_value, | ||
delta=est_delta, | ||
) | ||
elif isinstance(data_point, NumberDataPoint): | ||
self.assertEqual( | ||
data_point.value, | ||
expected_value, | ||
) | ||
|
||
self.assertDictEqual( | ||
expected_attributes, | ||
dict(data_point.attributes), | ||
) | ||
|
||
def test_basic_metric(self): | ||
start_time = default_timer() | ||
result = urllib.request.urlopen(self.URL) | ||
client_duration_estimated = (default_timer() - start_time) * 1000 | ||
|
||
metrics = self.get_sorted_metrics() | ||
self.assertEqual(len(metrics), 3) | ||
|
||
( | ||
client_duration, | ||
client_request_size, | ||
client_response_size, | ||
) = metrics[:3] | ||
|
||
self.assertEqual( | ||
client_duration.name, MetricInstruments.HTTP_CLIENT_DURATION | ||
) | ||
self.assert_metric_expected( | ||
client_duration, | ||
client_duration_estimated, | ||
{ | ||
"http.status_code": str(result.code), | ||
"http.method": "GET", | ||
"http.url": str(result.url), | ||
"http.flavor": "1.1", | ||
}, | ||
est_delta=200, | ||
) | ||
|
||
# net.peer.name | ||
|
||
self.assertEqual( | ||
client_request_size.name, | ||
MetricInstruments.HTTP_CLIENT_REQUEST_SIZE, | ||
) | ||
self.assert_metric_expected( | ||
client_request_size, | ||
0, | ||
{ | ||
"http.status_code": str(result.code), | ||
"http.method": "GET", | ||
"http.url": str(result.url), | ||
"http.flavor": "1.1", | ||
}, | ||
) | ||
|
||
self.assertEqual( | ||
client_response_size.name, | ||
MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE, | ||
) | ||
self.assert_metric_expected( | ||
client_response_size, | ||
result.length, | ||
{ | ||
"http.status_code": str(result.code), | ||
"http.method": "GET", | ||
"http.url": str(result.url), | ||
"http.flavor": "1.1", | ||
}, | ||
) | ||
|
||
def test_basic_metric_request_not_empty(self): | ||
data = {"header1": "value1", "header2": "value2"} | ||
data_encoded = urllib.parse.urlencode(data).encode() | ||
|
||
start_time = default_timer() | ||
result = urllib.request.urlopen(self.URL_POST, data=data_encoded) | ||
client_duration_estimated = (default_timer() - start_time) * 1000 | ||
|
||
metrics = self.get_sorted_metrics() | ||
self.assertEqual(len(metrics), 3) | ||
|
||
( | ||
client_duration, | ||
client_request_size, | ||
client_response_size, | ||
) = metrics[:3] | ||
|
||
self.assertEqual( | ||
client_duration.name, MetricInstruments.HTTP_CLIENT_DURATION | ||
) | ||
self.assert_metric_expected( | ||
client_duration, | ||
client_duration_estimated, | ||
{ | ||
"http.status_code": str(result.code), | ||
"http.method": "POST", | ||
"http.url": str(result.url), | ||
"http.flavor": "1.1", | ||
}, | ||
est_delta=200, | ||
) | ||
|
||
self.assertEqual( | ||
client_request_size.name, | ||
MetricInstruments.HTTP_CLIENT_REQUEST_SIZE, | ||
) | ||
self.assert_metric_expected( | ||
client_request_size, | ||
len(data_encoded), | ||
{ | ||
"http.status_code": str(result.code), | ||
"http.method": "POST", | ||
"http.url": str(result.url), | ||
"http.flavor": "1.1", | ||
}, | ||
) | ||
|
||
self.assertEqual( | ||
client_response_size.name, | ||
MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE, | ||
) | ||
self.assert_metric_expected( | ||
client_response_size, | ||
result.length, | ||
{ | ||
"http.status_code": str(result.code), | ||
"http.method": "POST", | ||
"http.url": str(result.url), | ||
"http.flavor": "1.1", | ||
}, | ||
) | ||
|
||
def test_metric_uninstrument(self): | ||
urllib.request.urlopen(self.URL) | ||
metrics = self.get_sorted_metrics() | ||
self.assertEqual(len(metrics), 3) | ||
|
||
URLLibInstrumentor().uninstrument() | ||
urllib.request.urlopen(self.URL) | ||
|
||
metrics = self.get_sorted_metrics() | ||
self.assertEqual(len(metrics), 3) | ||
|
||
for metric in metrics: | ||
for point in list(metric.data.data_points): | ||
self.assertEqual(point.count, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be a case where the result is None, but we should record the timing regardless of its return value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right this line should be outside the condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it so if the result is None we add records only for http_client_duration and http_client_request size,
the status code and HTTP flavor are missing from attributes because we take them from the response.
how does it sound?