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

Use is_recording flag in flask, django, tornado, boto, botocore instrumentations #1164

Merged
merged 10 commits into from
Sep 30, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ def collect_request_attributes(scope):

def set_status_code(span, status_code):
"""Adds HTTP response attributes to span using the status_code argument."""
if not span.is_recording():
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's worth adding a decorator as a convenience

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly? I think the repetitive-ness of this task just seems obvious because we just didn't do it in the beginning. However, new instrumentations should be respecting this anyways, so I don't think it's that much overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was thinking a decorator may help folks building future instrumentations group all the actions that should not occur when a span is not recording more cleanly 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be nice. There are some instrumentations that have some interim steps that always must be regardless of is_recording (which occur between some set_attr... methods). A decorator would be difficult to use in those cases.

return
try:
status_code = int(status_code)
except ValueError:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,27 +136,31 @@ def _common_request( # pylint: disable=too-many-locals
attributes={"endpoint": endpoint_name}
)

add_span_arg_tags(
span, endpoint_name, args, args_name, traced_args,
)
# Original func returns a boto.connection.HTTPResponse object
result = original_func(*args, **kwargs)

# Obtaining region name
region_name = _get_instance_region_name(instance)
if span.is_recording():
lzchen marked this conversation as resolved.
Show resolved Hide resolved
add_span_arg_tags(
span, endpoint_name, args, args_name, traced_args,
)

meta = {
"aws.agent": "boto",
"aws.operation": operation_name,
}
if region_name:
meta["aws.region"] = region_name
# Obtaining region name
region_name = _get_instance_region_name(instance)

for key, value in meta.items():
span.set_attribute(key, value)
meta = {
"aws.agent": "boto",
"aws.operation": operation_name,
}
if region_name:
meta["aws.region"] = region_name

# Original func returns a boto.connection.HTTPResponse object
result = original_func(*args, **kwargs)
span.set_attribute("http.status_code", getattr(result, "status"))
span.set_attribute("http.method", getattr(result, "_method"))
for key, value in meta.items():
span.set_attribute(key, value)

span.set_attribute(
"http.status_code", getattr(result, "status")
)
span.set_attribute("http.method", getattr(result, "_method"))

return result

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

from unittest import skipUnless
from unittest.mock import Mock, patch

import boto.awslambda
import boto.ec2
Expand Down Expand Up @@ -80,6 +81,23 @@ def test_ec2_client(self):
self.assertEqual(span.attributes["aws.region"], "us-west-2")
self.assertEqual(span.name, "ec2.command")

@mock_ec2_deprecated
def test_not_recording(self):
mock_tracer = Mock()
mock_span = Mock()
mock_span.is_recording.return_value = False
mock_tracer.start_span.return_value = mock_span
mock_tracer.use_span.return_value.__enter__ = mock_span
mock_tracer.use_span.return_value.__exit__ = mock_span
with patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
ec2 = boto.ec2.connect_to_region("us-west-2")
ec2.get_all_instances()
self.assertFalse(mock_span.is_recording())
self.assertTrue(mock_span.is_recording.called)
self.assertFalse(mock_span.set_attribute.called)
self.assertFalse(mock_span.set_status.called)

@mock_ec2_deprecated
def test_analytics_enabled_with_rate(self):
ec2 = boto.ec2.connect_to_region("us-west-2")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ def _patched_api_call(self, original_func, instance, args, kwargs):
) as span:

operation = None
if args:
if args and span.is_recording():
operation = args[0]
span.resource = Resource(
aabmass marked this conversation as resolved.
Show resolved Hide resolved
attributes={
Expand All @@ -119,25 +119,28 @@ def _patched_api_call(self, original_func, instance, args, kwargs):
{"params", "path", "verb"},
)

region_name = deep_getattr(instance, "meta.region_name")
if span.is_recording():
lzchen marked this conversation as resolved.
Show resolved Hide resolved
region_name = deep_getattr(instance, "meta.region_name")

meta = {
"aws.agent": "botocore",
"aws.operation": operation,
"aws.region": region_name,
}
for key, value in meta.items():
span.set_attribute(key, value)
meta = {
"aws.agent": "botocore",
"aws.operation": operation,
"aws.region": region_name,
}
for key, value in meta.items():
span.set_attribute(key, value)

result = original_func(*args, **kwargs)

span.set_attribute(
"http.status_code",
result["ResponseMetadata"]["HTTPStatusCode"],
)
span.set_attribute(
"retry_attempts", result["ResponseMetadata"]["RetryAttempts"],
)
if span.is_recording():
span.set_attribute(
"http.status_code",
result["ResponseMetadata"]["HTTPStatusCode"],
)
span.set_attribute(
"retry_attempts",
result["ResponseMetadata"]["RetryAttempts"],
)

return result

Expand Down Expand Up @@ -177,6 +180,9 @@ def flatten_dict(dict_, sep=".", prefix=""):
else {prefix: dict_}
)

if not span.is_recording():
return

if endpoint_name not in {"kms", "sts"}:
tags = dict(
(name, value)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
# 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.

from unittest.mock import Mock, patch

import botocore.session
from botocore.exceptions import ParamValidationError
from moto import ( # pylint: disable=import-error
Expand Down Expand Up @@ -61,6 +77,23 @@ def test_traced_client(self):
)
self.assertEqual(span.name, "ec2.command")

@mock_ec2
def test_not_recording(self):
mock_tracer = Mock()
mock_span = Mock()
mock_span.is_recording.return_value = False
mock_tracer.start_span.return_value = mock_span
mock_tracer.use_span.return_value.__enter__ = mock_span
mock_tracer.use_span.return_value.__exit__ = mock_span
with patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
ec2 = self.session.create_client("ec2", region_name="us-west-2")
ec2.describe_instances()
self.assertFalse(mock_span.is_recording())
self.assertTrue(mock_span.is_recording.called)
self.assertFalse(mock_span.set_attribute.called)
self.assertFalse(mock_span.set_status.called)

@mock_ec2
def test_traced_client_analytics(self):
ec2 = self.session.create_client("ec2", region_name="us-west-2")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,21 +101,23 @@ def process_request(self, request):

tracer = get_tracer(__name__, __version__)

attributes = collect_request_attributes(environ)
for attr in self._traced_request_attrs:
value = getattr(request, attr, None)
if value is not None:
attributes[attr] = str(value)

span = tracer.start_span(
self._get_span_name(request),
kind=SpanKind.SERVER,
attributes=attributes,
start_time=environ.get(
"opentelemetry-instrumentor-django.starttime_key"
),
)

if span.is_recording():
codeboten marked this conversation as resolved.
Show resolved Hide resolved
attributes = collect_request_attributes(environ)
for attr in self._traced_request_attrs:
value = getattr(request, attr, None)
if value is not None:
attributes[attr] = str(value)
Comment on lines +113 to +117
Copy link
Contributor

Choose a reason for hiding this comment

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

@owais created a nifty utils function that you can use like this

Suggested change
attributes = collect_request_attributes(environ)
for attr in self._traced_request_attrs:
value = getattr(request, attr, None)
if value is not None:
attributes[attr] = str(value)
attributes = collect_request_attributes(environ)
attributes = extract_attributes_from_object(
request, self._traced_request_attrs, attributes
)

It's still in PR though so no need to use it if this is ready first.

for key, value in attributes.items():
span.set_attribute(key, value)

activation = tracer.use_span(span, end_on_exit=True)
activation.__enter__()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

from sys import modules
from unittest.mock import patch
from unittest.mock import Mock, patch

from django import VERSION
from django.conf import settings
Expand Down Expand Up @@ -89,6 +89,21 @@ def test_traced_get(self):
self.assertEqual(span.attributes["http.status_code"], 200)
self.assertEqual(span.attributes["http.status_text"], "OK")

def test_not_recording(self):
mock_tracer = Mock()
mock_span = Mock()
mock_span.is_recording.return_value = False
mock_tracer.start_span.return_value = mock_span
mock_tracer.use_span.return_value.__enter__ = mock_span
mock_tracer.use_span.return_value.__exit__ = mock_span
with patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
Client().get("/traced/")
self.assertFalse(mock_span.is_recording())
self.assertTrue(mock_span.is_recording.called)
self.assertFalse(mock_span.set_attribute.called)
self.assertFalse(mock_span.set_status.called)

def test_traced_post(self):
Client().post("/traced/")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,17 +119,20 @@ def _before_request():

tracer = trace.get_tracer(__name__, __version__)

attributes = otel_wsgi.collect_request_attributes(environ)
if flask.request.url_rule:
# For 404 that result from no route found, etc, we
# don't have a url_rule.
attributes["http.route"] = flask.request.url_rule.rule
span = tracer.start_span(
span_name,
kind=trace.SpanKind.SERVER,
attributes=attributes,
start_time=environ.get(_ENVIRON_STARTTIME_KEY),
)
if span.is_recording():
attributes = otel_wsgi.collect_request_attributes(environ)
if flask.request.url_rule:
# For 404 that result from no route found, etc, we
# don't have a url_rule.
attributes["http.route"] = flask.request.url_rule.rule
for key, value in attributes.items():
span.set_attribute(key, value)

activation = tracer.use_span(span, end_on_exit=True)
activation.__enter__()
environ[_ENVIRON_ACTIVATION_KEY] = activation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from unittest.mock import patch
from unittest.mock import Mock, patch

from flask import Flask, request

Expand Down Expand Up @@ -106,6 +106,21 @@ def test_simple(self):
self.assertEqual(span_list[0].kind, trace.SpanKind.SERVER)
self.assertEqual(span_list[0].attributes, expected_attrs)

def test_not_recording(self):
mock_tracer = Mock()
mock_span = Mock()
mock_span.is_recording.return_value = False
mock_tracer.start_span.return_value = mock_span
mock_tracer.use_span.return_value.__enter__ = mock_span
mock_tracer.use_span.return_value.__exit__ = mock_span
with patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
self.client.get("/hello/123")
self.assertFalse(mock_span.is_recording())
self.assertTrue(mock_span.is_recording.called)
self.assertFalse(mock_span.set_attribute.called)
self.assertFalse(mock_span.set_status.called)

def test_404(self):
expected_attrs = expected_attributes(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,17 @@ def fetch_async(tracer, func, _, args, kwargs):
request = args[0]

span = tracer.start_span(
request.method,
kind=trace.SpanKind.CLIENT,
attributes={
request.method, kind=trace.SpanKind.CLIENT, start_time=start_time,
)

if span.is_recording():
attributes = {
"component": "tornado",
"http.url": request.url,
"http.method": request.method,
},
start_time=start_time,
)
}
for key, value in attributes.items():
span.set_attribute(key, value)

with tracer.use_span(span):
propagators.inject(type(request.headers).__setitem__, request.headers)
Expand All @@ -61,7 +63,7 @@ def _finish_tracing_callback(future, span):
status_code = None
description = None
exc = future.exception()
if exc:
if span.is_recording() and exc:
if isinstance(exc, HTTPError):
status_code = exc.code
description = "{}: {}".format(type(exc).__name__, exc)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.


from unittest.mock import patch
from unittest.mock import Mock, patch

from tornado.testing import AsyncHTTPTestCase

Expand Down Expand Up @@ -132,6 +132,21 @@ def _test_http_method_call(self, method):

self.memory_exporter.clear()

def test_not_recording(self):
mock_tracer = Mock()
mock_span = Mock()
mock_span.is_recording.return_value = False
mock_tracer.start_span.return_value = mock_span
mock_tracer.use_span.return_value.__enter__ = mock_span
mock_tracer.use_span.return_value.__exit__ = mock_span
with patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
self.fetch("/")
self.assertFalse(mock_span.is_recording())
self.assertTrue(mock_span.is_recording.called)
self.assertFalse(mock_span.set_attribute.called)
self.assertFalse(mock_span.set_status.called)

def test_async_handler(self):
self._test_async_handler("/async", "AsyncHandler")

Expand Down