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

Adding metric collection as part of instrumentations - Requests #1116

Merged
merged 25 commits into from
Sep 25, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
37 changes: 37 additions & 0 deletions docs/examples/basic_meter/standard.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# 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.
#
"""
This module shows how you can enable collection and exporting of standard
metrics related to instrumentations.
lzchen marked this conversation as resolved.
Show resolved Hide resolved
"""
import requests

from opentelemetry import metrics
from opentelemetry.instrumentation.requests import RequestsInstrumentor
from opentelemetry.sdk.metrics import MeterProvider
from opentelemetry.sdk.metrics.export import ConsoleMetricsExporter

# Sets the global MeterProvider instance
metrics.set_meter_provider(MeterProvider())

# Exporter to export metrics to the console
exporter = ConsoleMetricsExporter()

RequestsInstrumentor().instrument(
metrics_exporter=exporter, metrics_interval=5
)
response = requests.get("http://example.com")

input("...\n")
1 change: 1 addition & 0 deletions docs/instrumentation/instrumentation.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ Submodules
:maxdepth: 1

instrumentor
metric
7 changes: 7 additions & 0 deletions docs/instrumentation/metric.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
opentelemetry.instrumentation.metric package
============================================

.. automodule:: opentelemetry.instrumentation.metric
:members:
:undoc-members:
:show-inheritance:
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
([#1040](https://github.com/open-telemetry/opentelemetry-python/pull/1040))
- Drop support for Python 3.4
([#1099](https://github.com/open-telemetry/opentelemetry-python/pull/1099))
- Add support for standard metrics - duration
lzchen marked this conversation as resolved.
Show resolved Hide resolved
([#1116](https://github.com/open-telemetry/opentelemetry-python/pull/1116))

## Version 0.12b0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import opentelemetry.instrumentation.requests

# You can optionally pass a custom TracerProvider to
RequestInstrumentor.instrument()
# RequestInstrumentor.instrument()
opentelemetry.instrumentation.requests.RequestsInstrumentor().instrument()
response = requests.get(url="https://www.example.org/")

Expand All @@ -43,6 +43,10 @@

from opentelemetry import context, propagators
from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
from opentelemetry.instrumentation.metric import (
HTTPMetricRecorder,
MetricMixin,
)
from opentelemetry.instrumentation.requests.version import __version__
from opentelemetry.instrumentation.utils import http_status_to_canonical_code
from opentelemetry.trace import SpanKind, get_tracer
Expand All @@ -54,6 +58,7 @@


# pylint: disable=unused-argument
# pylint: disable=R0915
def _instrument(tracer_provider=None, span_callback=None):
"""Enables tracing of all requests calls that go through
:code:`requests.session.Session.request` (this includes
Expand Down Expand Up @@ -118,43 +123,63 @@ def _instrumented_requests_call(

exception = None

recorder = RequestsInstrumentor().metric_recorder

labels = {}
labels["http.method"] = method
labels["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 it possible to infer http.scheme as well here using url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


with get_tracer(
__name__, __version__, tracer_provider
).start_as_current_span(span_name, kind=SpanKind.CLIENT) as span:
if span.is_recording():
span.set_attribute("component", "http")
span.set_attribute("http.method", method.upper())
span.set_attribute("http.url", url)

headers = get_or_create_headers()
propagators.inject(type(headers).__setitem__, headers)

token = context.attach(
context.set_value(_SUPPRESS_REQUESTS_INSTRUMENTATION_KEY, True)
)
try:
result = call_wrapped() # *** PROCEED
except Exception as exc: # pylint: disable=W0703
exception = exc
result = getattr(exc, "response", None)
finally:
context.detach(token)

if exception is not None and span.is_recording():
span.set_status(
Status(_exception_to_canonical_code(exception))
with recorder.record_duration(labels):
if span.is_recording():
span.set_attribute("component", "http")
span.set_attribute("http.method", method)
span.set_attribute("http.url", url)

headers = get_or_create_headers()
propagators.inject(type(headers).__setitem__, headers)

token = context.attach(
context.set_value(
_SUPPRESS_REQUESTS_INSTRUMENTATION_KEY, True
)
)
span.record_exception(exception)

if result is not None and span.is_recording():
span.set_attribute("http.status_code", result.status_code)
span.set_attribute("http.status_text", result.reason)
span.set_status(
Status(http_status_to_canonical_code(result.status_code))
)

if span_callback is not None:
span_callback(span, result)
try:
result = call_wrapped() # *** PROCEED
except Exception as exc: # pylint: disable=W0703
exception = exc
result = getattr(exc, "response", None)
finally:
context.detach(token)

if exception is not None and span.is_recording():
span.set_status(
Status(_exception_to_canonical_code(exception))
)
span.record_exception(exception)

if result is not None:
if span.is_recording():
span.set_attribute(
"http.status_code", result.status_code
)
span.set_attribute("http.status_text", result.reason)
span.set_status(
Status(
http_status_to_canonical_code(
result.status_code
)
)
)
labels["http.status_code"] = result.status_code
labels["http.status_text"] = result.reason
if result.raw and result.raw.version:
labels["http.flavor"] = result.raw.version

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

if exception is not None:
raise exception.with_traceback(exception.__traceback__)
Expand Down Expand Up @@ -202,7 +227,7 @@ def _exception_to_canonical_code(exc: Exception) -> StatusCanonicalCode:
return StatusCanonicalCode.UNKNOWN


class RequestsInstrumentor(BaseInstrumentor):
class RequestsInstrumentor(BaseInstrumentor, MetricMixin):
"""An instrumentor for requests
See `BaseInstrumentor`
"""
Expand All @@ -214,11 +239,21 @@ def _instrument(self, **kwargs):
**kwargs: Optional arguments
``tracer_provider``: a TracerProvider, defaults to global
``span_callback``: An optional callback invoked before returning the http response. Invoked with Span and requests.Response
``metrics_exporter``: the metric exporter to send requests related metrics to.
``metrics_interval``: the export interval for the metric exporter
"""
_instrument(
tracer_provider=kwargs.get("tracer_provider"),
span_callback=kwargs.get("span_callback"),
)
self.init_metrics(
__name__,
__version__,
kwargs.get("metrics_exporter"),
kwargs.get("metrics_interval"),
)
# pylint: disable=W0201
self.metric_recorder = HTTPMetricRecorder(self.meter, SpanKind.CLIENT)

def _uninstrument(self, **kwargs):
_uninstrument()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from opentelemetry import context, propagators, trace
from opentelemetry.instrumentation.requests import RequestsInstrumentor
from opentelemetry.sdk import resources
from opentelemetry.sdk.util import get_dict_as_key
from opentelemetry.test.mock_textmap import MockTextMapPropagator
from opentelemetry.test.test_base import TestBase
from opentelemetry.trace.status import StatusCanonicalCode
Expand Down Expand Up @@ -88,6 +89,27 @@ def test_basic(self):
span, opentelemetry.instrumentation.requests
)

self.assertIsNotNone(RequestsInstrumentor().meter)
self.assertEqual(len(RequestsInstrumentor().meter.metrics), 1)
recorder = RequestsInstrumentor().meter.metrics.pop()
match_key = get_dict_as_key(
{
"http.flavor": 11,
lzchen marked this conversation as resolved.
Show resolved Hide resolved
"http.method": "GET",
"http.status_code": 200,
lzchen marked this conversation as resolved.
Show resolved Hide resolved
"http.status_text": "OK",
"http.url": "http://httpbin.org/status/200",
}
)
for key in recorder.bound_instruments.keys():
self.assertEqual(key, match_key)
# pylint: disable=protected-access
bound = recorder.bound_instruments.get(key)
for view_data in bound.view_datas:
self.assertEqual(view_data.labels, key)
self.assertEqual(view_data.aggregator.current.count, 1)
self.assertGreater(view_data.aggregator.current.sum, 0)

def test_not_foundbasic(self):
url_404 = "http://httpbin.org/status/404"
httpretty.register_uri(
Expand Down Expand Up @@ -246,6 +268,23 @@ def test_requests_exception_without_response(self, *_, **__):
span.status.canonical_code, StatusCanonicalCode.UNKNOWN
)

self.assertIsNotNone(RequestsInstrumentor().meter)
self.assertEqual(len(RequestsInstrumentor().meter.metrics), 1)
recorder = RequestsInstrumentor().meter.metrics.pop()
match_key = get_dict_as_key(
{
"http.method": "GET",
"http.url": "http://httpbin.org/status/200",
}
)
for key in recorder.bound_instruments.keys():
self.assertEqual(key, match_key)
# pylint: disable=protected-access
bound = recorder.bound_instruments.get(key)
for view_data in bound.view_datas:
self.assertEqual(view_data.labels, key)
self.assertEqual(view_data.aggregator.current.count, 1)

mocked_response = requests.Response()
mocked_response.status_code = 500
mocked_response.reason = "Internal Server Error"
Expand All @@ -272,6 +311,24 @@ def test_requests_exception_with_response(self, *_, **__):
self.assertEqual(
span.status.canonical_code, StatusCanonicalCode.INTERNAL
)
self.assertIsNotNone(RequestsInstrumentor().meter)
self.assertEqual(len(RequestsInstrumentor().meter.metrics), 1)
recorder = RequestsInstrumentor().meter.metrics.pop()
match_key = get_dict_as_key(
{
"http.method": "GET",
"http.status_code": 500,
"http.status_text": "Internal Server Error",
"http.url": "http://httpbin.org/status/200",
}
)
for key in recorder.bound_instruments.keys():
self.assertEqual(key, match_key)
# pylint: disable=protected-access
bound = recorder.bound_instruments.get(key)
for view_data in bound.view_datas:
self.assertEqual(view_data.labels, key)
self.assertEqual(view_data.aggregator.current.count, 1)

@mock.patch("requests.adapters.HTTPAdapter.send", side_effect=Exception)
def test_requests_basic_exception(self, *_, **__):
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-instrumentation/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

- Drop support for Python 3.4
([#1099](https://github.com/open-telemetry/opentelemetry-python/pull/1099))
- Add support for standard metrics - duration
([#1116](https://github.com/open-telemetry/opentelemetry-python/pull/1116))

## 0.9b0

Expand Down
1 change: 1 addition & 0 deletions opentelemetry-instrumentation/setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ zip_safe = False
include_package_data = True
install_requires =
opentelemetry-api == 0.13dev0
opentelemetry-sdk == 0.13dev0
wrapt >= 1.0.0, < 2.0.0

[options.packages.find]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# 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.
# type: ignore

"""
OpenTelemetry Instrumentation Metric mixin
"""
from contextlib import contextmanager
from time import time

from opentelemetry import metrics
from opentelemetry.sdk.metrics import PushController, ValueRecorder
from opentelemetry.trace import SpanKind


class MetricMixin:
"""Used to record metrics related to instrumentations."""

def init_metrics(self, name, version, exporter=None, interval=None):
lzchen marked this conversation as resolved.
Show resolved Hide resolved
self._meter = metrics.get_meter(name, version)
if exporter and interval:
self._controller = PushController(
meter=self._meter, exporter=exporter, interval=interval
)
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about only exposing the meter property and removing this code, so users would have to do:

requests_instrumentor = RequestsInstrumentor(...)
metrics.get_meter_provider().start_pipeline(requests_instrumentor.meter, exporter, 5)

Then you can remove the opentelemetry-sdk dependency for the instrumentation. This might be overly pedantic, but depending on the opentelemetry-sdk prevents people from using a third-party SDK (if one ever existed).

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 was actually thinking about this approach originally, just expose the meter so users can decide what they want to do with it. I'm okay with t removing the automatically started pipeline. However, this feature requires the dependency on opentelemetry-sdk regardless, because of line 63 in metric.py in opentelemetry-instrumentation. To enable this, the sdk implementation of ValueRecorder must be used, so we MUST have a dependency on it. I think the message is: if you want to autocollect these metrics, this will be a feature offered by the default sdk and so you have to install it. I also don't know if the api and sdk separation of metrics makes sense today. Will we ever even have a different implementation of the metrics sdk? It's not really the same as the tracer, in which we would.

If there is a big push to not take a dependency on the sdk, we would probably have to change our metrics api implementation and create an explicit create_value_recorder method for the meter s. I'm actually okay with this, but might want to leave this for another pr. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

If there is a big push to not take a dependency on the sdk, we would probably have to change our metrics api implementation and create an explicit create_value_recorder method for the meter s. I'm actually okay with this, but might want to leave this for another pr. Thoughts?

Ya I agree it would be easy to work around that use of ValueRecorder later if we decide the instrumentation shouldn't take dependency on the SDK (it would be breaking change to the Instrumentor constructor though?). Could you explain why the api/sdk separation isn't the same with metrics as it is with tracer (I think I'm missing some background info)?

Would be great to get other Python SIG folks' thoughts on this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain why the api/sdk separation isn't the same with metrics as it is with tracer

I feel like we don't expect people to implement their own sdk to record + export metrics. It's as if we introduced the api + sdk separation simply to match what trace is doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

This discussion makes me wonder if the behaviour exposed in the ValueRecorder by the SDK should just be in the API then, instead of having an interface that is not usable with the SDK. If this isn't already being discussed in the metrics SIG, it should, I would suspect other SIGs are running into similar issues.

Copy link
Contributor Author

@lzchen lzchen Sep 25, 2020

Choose a reason for hiding this comment

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

Java implementation and Go implementation both have constructors for each metric instrument. So depending on whether they are using the sdk MeterProvider or not, it returns different implementations of the instrument, so they have an interface that is useable by the SDK. We should probably do something similar.


@property
def meter(self):
return self._meter


class MetricRecorder:
aabmass marked this conversation as resolved.
Show resolved Hide resolved
"""Base class for metric recorders of different types."""

def __init__(self, meter):
lzchen marked this conversation as resolved.
Show resolved Hide resolved
self._meter = meter


class HTTPMetricRecorder(MetricRecorder):
"""Metric recorder for http instrumentations. Tracks duration."""

def __init__(self, meter, kind: SpanKind):
lzchen marked this conversation as resolved.
Show resolved Hide resolved
super().__init__(meter)
self.kind = kind
if self._meter:
self._duration = self._meter.create_metric(
name="{}.{}.duration".format("http", self.kind.name.lower()),
description="measures the duration of the {} HTTP request".format(
"inbound" if self.kind is SpanKind.SERVER else "outbound"
),
unit="ms",
value_type=float,
metric_type=ValueRecorder,
)

@contextmanager
def record_duration(self, labels):
aabmass marked this conversation as resolved.
Show resolved Hide resolved
start_time = time()
try:
yield start_time
finally:
if self._meter:
elapsed_time = (time() - start_time) * 1000
self._duration.record(elapsed_time, labels)
Loading