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

Metric instrumentation asgi #1197

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
a01f115
add metric instrumentation changes I
TheAnshul756 Jul 18, 2022
1cbf9eb
add metric instrumentation in asgi
TheAnshul756 Jul 19, 2022
8a452c0
add change log
TheAnshul756 Jul 19, 2022
5bf7fb9
fix lint
TheAnshul756 Jul 19, 2022
210dbbd
Merge branch 'main' into metric-instrumentation-asgi
lzchen Jul 20, 2022
67258fa
Merge branch 'main' into metric-instrumentation-asgi
ocelotl Jul 22, 2022
d2df8b1
change time duration start time position for metric
TheAnshul756 Jul 27, 2022
c4d2b73
Merge branch 'metric-instrumentation-asgi' of https://github.com/TheA…
TheAnshul756 Jul 27, 2022
f4b8962
add basic success test
TheAnshul756 Jul 28, 2022
0874f23
remove metric instrumentation for websockets
TheAnshul756 Jul 29, 2022
6819509
Merge branch 'main' into metric-instrumentation-asgi
ashu658 Aug 1, 2022
5b951d9
Merge branch 'main' of https://github.com/TheAnshul756/opentelemetry-…
TheAnshul756 Aug 1, 2022
363242c
Merge branch 'metric-instrumentation-asgi' of https://github.com/TheA…
TheAnshul756 Aug 1, 2022
1aef36e
add value test and websocket test for metric
TheAnshul756 Aug 2, 2022
838a165
Merge branch 'main' into metric-instrumentation-asgi
srikanthccv Aug 4, 2022
d04ebbf
Merge branch 'main' into metric-instrumentation-asgi
srikanthccv Aug 4, 2022
017319a
move attributes to utils
TheAnshul756 Aug 5, 2022
d7fa928
resolve conflict
TheAnshul756 Aug 5, 2022
e04ed38
fix error
TheAnshul756 Aug 8, 2022
45c8fb5
fix lint
TheAnshul756 Aug 8, 2022
25a888f
fix lint
TheAnshul756 Aug 8, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `opentelemetry-instrumentation-redis` add support to instrument RedisCluster clients
([#1177](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1177))
- `opentelemetry-instrumentation-sqlalchemy` Added span for the connection phase ([#1133](https://github.com/open-telemetry/opentelemetry-python-contrib/issues/1133))
- Add metric instrumentation in asgi
([#1197](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1197))

## [1.12.0rc2-0.32b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.12.0rc2-0.32b0) - 2022-07-01

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ def client_response_hook(span: Span, message: dict):
import typing
import urllib
from functools import wraps
from timeit import default_timer
from typing import Tuple

from asgiref.compatibility import guarantee_single_callable
Expand All @@ -162,6 +163,7 @@ def client_response_hook(span: Span, message: dict):
_start_internal_or_server_span,
http_status_to_status_code,
)
from opentelemetry.metrics import get_meter
from opentelemetry.propagators.textmap import Getter, Setter
from opentelemetry.semconv.trace import SpanAttributes
from opentelemetry.trace import Span, set_span_in_context
Expand All @@ -179,6 +181,26 @@ def client_response_hook(span: Span, message: dict):
_ClientRequestHookT = typing.Optional[typing.Callable[[Span, dict], None]]
_ClientResponseHookT = typing.Optional[typing.Callable[[Span, dict], None]]

# List of recommended attributes
_duration_attrs = [
SpanAttributes.HTTP_METHOD,
SpanAttributes.HTTP_HOST,
SpanAttributes.HTTP_SCHEME,
SpanAttributes.HTTP_STATUS_CODE,
SpanAttributes.HTTP_FLAVOR,
SpanAttributes.HTTP_SERVER_NAME,
SpanAttributes.NET_HOST_NAME,
SpanAttributes.NET_HOST_PORT,
]

_active_requests_count_attrs = [
SpanAttributes.HTTP_METHOD,
SpanAttributes.HTTP_HOST,
SpanAttributes.HTTP_SCHEME,
SpanAttributes.HTTP_FLAVOR,
SpanAttributes.HTTP_SERVER_NAME,
]
TheAnshul756 marked this conversation as resolved.
Show resolved Hide resolved


class ASGIGetter(Getter[dict]):
def get(
Expand Down Expand Up @@ -361,6 +383,22 @@ def get_default_span_details(scope: dict) -> Tuple[str, dict]:
return span_name, {}


def _parse_active_request_count_attrs(req_attrs):
active_requests_count_attrs = {}
for attr_key in _active_requests_count_attrs:
if req_attrs.get(attr_key) is not None:
active_requests_count_attrs[attr_key] = req_attrs[attr_key]
return active_requests_count_attrs


def _parse_duration_attrs(req_attrs):
duration_attrs = {}
for attr_key in _duration_attrs:
if req_attrs.get(attr_key) is not None:
duration_attrs[attr_key] = req_attrs[attr_key]
return duration_attrs
TheAnshul756 marked this conversation as resolved.
Show resolved Hide resolved


class OpenTelemetryMiddleware:
"""The ASGI application middleware.

Expand Down Expand Up @@ -391,9 +429,21 @@ def __init__(
client_request_hook: _ClientRequestHookT = None,
client_response_hook: _ClientResponseHookT = None,
tracer_provider=None,
meter_provider=None,
):
self.app = guarantee_single_callable(app)
self.tracer = trace.get_tracer(__name__, __version__, tracer_provider)
self.meter = get_meter(__name__, __version__, meter_provider)
self.duration_histogram = self.meter.create_histogram(
name="http.server.duration",
unit="ms",
description="measures the duration of the inbound HTTP request",
)
self.active_requests_counter = self.meter.create_up_down_counter(
name="http.server.active_requests",
unit="requests",
description="measures the number of concurrent HTTP requests that are currently in-flight",
)
self.excluded_urls = excluded_urls
self.default_span_details = (
default_span_details or get_default_span_details
Expand Down Expand Up @@ -426,12 +476,17 @@ async def __call__(self, scope, receive, send):
context_carrier=scope,
context_getter=asgi_getter,
)

attributes = collect_request_attributes(scope)
attributes.update(additional_attributes)
active_requests_count_attrs = _parse_active_request_count_attrs(
attributes
)
duration_attrs = _parse_duration_attrs(attributes)
start = default_timer()
self.active_requests_counter.add(1, active_requests_count_attrs)
try:
with trace.use_span(span, end_on_exit=True) as current_span:
if current_span.is_recording():
attributes = collect_request_attributes(scope)
attributes.update(additional_attributes)
for key, value in attributes.items():
current_span.set_attribute(key, value)

Expand All @@ -454,10 +509,14 @@ async def __call__(self, scope, receive, send):
span_name,
scope,
send,
duration_attrs,
)

await self.app(scope, otel_receive, otel_send)
finally:
duration = max(round((default_timer() - start) * 1000), 0)
self.duration_histogram.record(duration, duration_attrs)
self.active_requests_counter.add(-1, active_requests_count_attrs)
if token:
context.detach(token)

Expand All @@ -478,7 +537,9 @@ async def otel_receive():

return otel_receive

def _get_otel_send(self, server_span, server_span_name, scope, send):
def _get_otel_send(
self, server_span, server_span_name, scope, send, duration_attrs
):
@wraps(send)
async def otel_send(message):
with self.tracer.start_as_current_span(
Expand All @@ -489,6 +550,9 @@ async def otel_send(message):
if send_span.is_recording():
if message["type"] == "http.response.start":
status_code = message["status"]
duration_attrs[
SpanAttributes.HTTP_STATUS_CODE
] = status_code
set_status_code(server_span, status_code)
set_status_code(send_span, status_code)
elif message["type"] == "websocket.send":
lzchen marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@
set_global_response_propagator,
)
from opentelemetry.sdk import resources
from opentelemetry.sdk.metrics.export import (
HistogramDataPoint,
NumberDataPoint,
)
from opentelemetry.semconv.trace import SpanAttributes
from opentelemetry.test.asgitestutil import (
AsgiTestBase,
Expand All @@ -36,6 +40,15 @@
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE,
)

_expected_metric_names = [
"http.server.active_requests",
"http.server.duration",
]
_recommended_attrs = {
"http.server.active_requests": otel_asgi._active_requests_count_attrs,
"http.server.duration": otel_asgi._duration_attrs,
}


async def http_app(scope, receive, send):
message = await receive()
Expand Down Expand Up @@ -523,6 +536,38 @@ def update_expected_hook_results(expected):
outputs, modifiers=[update_expected_hook_results]
)

def test_asgi_metrics(self):
app = otel_asgi.OpenTelemetryMiddleware(simple_asgi)
self.seed_app(app)
self.send_default_request()
self.seed_app(app)
self.send_default_request()
self.seed_app(app)
self.send_default_request()
metrics_list = self.memory_metrics_reader.get_metrics_data()
number_data_point_seen = False
histogram_data_point_seen = False
self.assertTrue(len(metrics_list.resource_metrics) != 0)
for resource_metric in metrics_list.resource_metrics:
self.assertTrue(len(resource_metric.scope_metrics) != 0)
for scope_metric in resource_metric.scope_metrics:
self.assertTrue(len(scope_metric.metrics) != 0)
for metric in scope_metric.metrics:
self.assertIn(metric.name, _expected_metric_names)
data_points = list(metric.data.data_points)
self.assertEqual(len(data_points), 1)
for point in data_points:
if isinstance(point, HistogramDataPoint):
self.assertEqual(point.count, 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test out the values?

Copy link
Member

Choose a reason for hiding this comment

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

I see that the same test from the WSGI metrics instrumentation style is copied for writing tests everywhere. I would like to see the actual values as well with more tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added test to check the attributes values. Please have a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not referring to the values of the attributes. I am referring to the values of the data points recorded in the data points. Like sum for histogram data point and value for number data point. We are trying to verify that the duration and number of requests recorded are what we expect.

histogram_data_point_seen = True
if isinstance(point, NumberDataPoint):
number_data_point_seen = True
for attr in point.attributes:
self.assertIn(
attr, _recommended_attrs[metric.name]
)
self.assertTrue(number_data_point_seen and histogram_data_point_seen)


TheAnshul756 marked this conversation as resolved.
Show resolved Hide resolved
class TestAsgiAttributes(unittest.TestCase):
def setUp(self):
Expand Down