From 535c2e614f7a772e132cdda24d14bf6c1ad5dbdb Mon Sep 17 00:00:00 2001 From: Leighton Chen Date: Wed, 14 Oct 2020 15:07:23 -0400 Subject: [PATCH] Use is_recording flag in jinja, celery, esearch, falcon instrumentations (#1241) --- .../instrumentation/celery/__init__.py | 22 ++++----- .../instrumentation/celery/utils.py | 2 + .../tests/test_utils.py | 24 ++++++++++ .../instrumentation/elasticsearch/__init__.py | 45 ++++++++++--------- .../tests/test_elasticsearch.py | 18 ++++++++ .../instrumentation/falcon/__init__.py | 13 +++--- .../tests/test_falcon.py | 17 ++++++- .../instrumentation/jinja2/__init__.py | 33 ++++++++------ .../tests/test_jinja2.py | 16 +++++++ .../instrumentation/pymemcache/__init__.py | 15 ++++--- .../instrumentation/tornado/__init__.py | 22 +++++---- 11 files changed, 160 insertions(+), 67 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py b/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py index 3bd912c5ed0..ab0cdf39b10 100644 --- a/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py @@ -148,10 +148,11 @@ def _trace_postrun(*args, **kwargs): return # request context tags - span.set_attribute(_TASK_TAG_KEY, _TASK_RUN) - utils.set_attributes_from_context(span, kwargs) - utils.set_attributes_from_context(span, task.request) - span.set_attribute(_TASK_NAME_KEY, task.name) + if span.is_recording(): + span.set_attribute(_TASK_TAG_KEY, _TASK_RUN) + utils.set_attributes_from_context(span, kwargs) + utils.set_attributes_from_context(span, task.request) + span.set_attribute(_TASK_NAME_KEY, task.name) activation.__exit__(None, None, None) utils.detach_span(task, task_id) @@ -169,10 +170,11 @@ def _trace_before_publish(self, *args, **kwargs): ) # apply some attributes here because most of the data is not available - span.set_attribute(_TASK_TAG_KEY, _TASK_APPLY_ASYNC) - span.set_attribute(_MESSAGE_ID_ATTRIBUTE_NAME, task_id) - span.set_attribute(_TASK_NAME_KEY, task.name) - utils.set_attributes_from_context(span, kwargs) + if span.is_recording(): + span.set_attribute(_TASK_TAG_KEY, _TASK_APPLY_ASYNC) + span.set_attribute(_MESSAGE_ID_ATTRIBUTE_NAME, task_id) + span.set_attribute(_TASK_NAME_KEY, task.name) + utils.set_attributes_from_context(span, kwargs) activation = self._tracer.use_span(span, end_on_exit=True) activation.__enter__() @@ -209,7 +211,7 @@ def _trace_failure(*args, **kwargs): # retrieve and pass exception info to activation span, _ = utils.retrieve_span(task, task_id) - if span is None: + if span is None or not span.is_recording(): return status_kwargs = {"canonical_code": StatusCanonicalCode.UNKNOWN} @@ -238,7 +240,7 @@ def _trace_retry(*args, **kwargs): return span, _ = utils.retrieve_span(task, task_id) - if span is None: + if span is None or not span.is_recording(): return # Add retry reason metadata to span diff --git a/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/utils.py b/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/utils.py index 60fe52f04ef..9a441345981 100644 --- a/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/utils.py +++ b/instrumentation/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/utils.py @@ -48,6 +48,8 @@ # pylint:disable=too-many-branches def set_attributes_from_context(span, context): """Helper to extract meta values from a Celery Context""" + if not span.is_recording(): + return for key in CELERY_CONTEXT_ATTRIBUTES: value = context.get(key) diff --git a/instrumentation/opentelemetry-instrumentation-celery/tests/test_utils.py b/instrumentation/opentelemetry-instrumentation-celery/tests/test_utils.py index 3df0f445b37..f48f06aafbe 100644 --- a/instrumentation/opentelemetry-instrumentation-celery/tests/test_utils.py +++ b/instrumentation/opentelemetry-instrumentation-celery/tests/test_utils.py @@ -69,6 +69,30 @@ def test_set_attributes_from_context(self): ) self.assertNotIn("custom_meta", span.attributes) + def test_set_attributes_not_recording(self): + # it should extract only relevant keys + context = { + "correlation_id": "44b7f305", + "delivery_info": {"eager": True}, + "eta": "soon", + "expires": "later", + "hostname": "localhost", + "id": "44b7f305", + "reply_to": "44b7f305", + "retries": 4, + "timelimit": ("now", "later"), + "custom_meta": "custom_value", + "routing_key": "celery", + } + + mock_span = mock.Mock() + mock_span.is_recording.return_value = False + utils.set_attributes_from_context(mock_span, context) + 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_set_attributes_from_context_empty_keys(self): # it should not extract empty keys context = { diff --git a/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py b/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py index f350a7dc280..6e9f411f8a4 100644 --- a/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-elasticsearch/src/opentelemetry/instrumentation/elasticsearch/__init__.py @@ -110,6 +110,7 @@ def _uninstrument(self, **kwargs): def _wrap_perform_request(tracer, span_name_prefix): + # pylint: disable=R0912 def wrapper(wrapped, _, args, kwargs): method = url = None try: @@ -125,26 +126,27 @@ def wrapper(wrapped, _, args, kwargs): params = kwargs.get("params", {}) body = kwargs.get("body", None) - attributes = { - "component": "elasticsearch-py", - "db.type": "elasticsearch", - } - - if url: - attributes["elasticsearch.url"] = url - if method: - attributes["elasticsearch.method"] = method - if body: - attributes["db.statement"] = str(body) - if params: - attributes["elasticsearch.params"] = str(params) - with tracer.start_as_current_span( - op_name, kind=SpanKind.CLIENT, attributes=attributes + op_name, kind=SpanKind.CLIENT, ) as span: + if span.is_recording(): + attributes = { + "component": "elasticsearch-py", + "db.type": "elasticsearch", + } + if url: + attributes["elasticsearch.url"] = url + if method: + attributes["elasticsearch.method"] = method + if body: + attributes["db.statement"] = str(body) + if params: + attributes["elasticsearch.params"] = str(params) + for key, value in attributes.items(): + span.set_attribute(key, value) try: rv = wrapped(*args, **kwargs) - if isinstance(rv, dict): + if isinstance(rv, dict) and span.is_recording(): for member in _ATTRIBUTES_FROM_RESULT: if member in rv: span.set_attribute( @@ -153,11 +155,12 @@ def wrapper(wrapped, _, args, kwargs): ) return rv except Exception as ex: # pylint: disable=broad-except - if isinstance(ex, elasticsearch.exceptions.NotFoundError): - status = StatusCanonicalCode.NOT_FOUND - else: - status = StatusCanonicalCode.UNKNOWN - span.set_status(Status(status, str(ex))) + if span.is_recording(): + if isinstance(ex, elasticsearch.exceptions.NotFoundError): + status = StatusCanonicalCode.NOT_FOUND + else: + status = StatusCanonicalCode.UNKNOWN + span.set_status(Status(status, str(ex))) raise ex return wrapper diff --git a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py index cc1d3147742..3d93838fe81 100644 --- a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py +++ b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py @@ -88,6 +88,24 @@ def test_instrumentor(self, request_mock): spans_list = self.get_ordered_finished_spans() self.assertEqual(len(spans_list), 1) + def test_span_not_recording(self, request_mock): + request_mock.return_value = (1, {}, {}) + mock_tracer = mock.Mock() + mock_span = mock.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 mock.patch("opentelemetry.trace.get_tracer") as tracer: + tracer.return_value = mock_tracer + Elasticsearch() + 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) + + ElasticsearchInstrumentor().uninstrument() + def test_prefix_arg(self, request_mock): prefix = "prefix-from-env" ElasticsearchInstrumentor().uninstrument() diff --git a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py index bfcd45a8b58..0a93fe01388 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py @@ -117,13 +117,16 @@ def __call__(self, env, start_response): token = context.attach( propagators.extract(otel_wsgi.get_header_from_environ, env) ) - attributes = otel_wsgi.collect_request_attributes(env) span = self._tracer.start_span( otel_wsgi.get_default_span_name(env), kind=trace.SpanKind.SERVER, - attributes=attributes, start_time=start_time, ) + if span.is_recording(): + attributes = otel_wsgi.collect_request_attributes(env) + for key, value in attributes.items(): + span.set_attribute(key, value) + activation = self._tracer.use_span(span, end_on_exit=True) activation.__enter__() env[_ENVIRON_SPAN_KEY] = span @@ -162,7 +165,7 @@ def __init__(self, tracer=None, traced_request_attrs=None): def process_request(self, req, resp): span = req.env.get(_ENVIRON_SPAN_KEY) - if not span: + if not span or not span.is_recording(): return attributes = extract_attributes_from_object( @@ -173,7 +176,7 @@ def process_request(self, req, resp): def process_resource(self, req, resp, resource, params): span = req.env.get(_ENVIRON_SPAN_KEY) - if not span: + if not span or not span.is_recording(): return resource_name = resource.__class__.__name__ @@ -186,7 +189,7 @@ def process_response( self, req, resp, resource, req_succeeded=None ): # pylint:disable=R0201 span = req.env.get(_ENVIRON_SPAN_KEY) - if not span: + if not span or not span.is_recording(): return status = resp.status diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py index 5e27e4aacbd..d64154a7770 100644 --- a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py +++ b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py @@ -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 falcon import testing @@ -188,3 +188,18 @@ def test_traced_request_attributes(self): span = self.memory_exporter.get_finished_spans()[0] self.assertIn("query_string", span.attributes) self.assertEqual(span.attributes["query_string"], "q=abc") + + def test_traced_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().simulate_get(path="/hello?q=abc") + 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) diff --git a/instrumentation/opentelemetry-instrumentation-jinja2/src/opentelemetry/instrumentation/jinja2/__init__.py b/instrumentation/opentelemetry-instrumentation-jinja2/src/opentelemetry/instrumentation/jinja2/__init__.py index 8ad883b279b..4123f7de526 100644 --- a/instrumentation/opentelemetry-instrumentation-jinja2/src/opentelemetry/instrumentation/jinja2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-jinja2/src/opentelemetry/instrumentation/jinja2/__init__.py @@ -78,39 +78,44 @@ def wrapper(wrapped, instance, args, kwargs): def _wrap_render(tracer, wrapped, instance, args, kwargs): """Wrap `Template.render()` or `Template.generate()` """ - template_name = instance.name or DEFAULT_TEMPLATE_NAME - attributes = {ATTRIBUTE_JINJA2_TEMPLATE_NAME: template_name} with tracer.start_as_current_span( - "jinja2.render", kind=SpanKind.INTERNAL, attributes=attributes - ): + "jinja2.render", kind=SpanKind.INTERNAL, + ) as span: + if span.is_recording(): + template_name = instance.name or DEFAULT_TEMPLATE_NAME + span.set_attribute(ATTRIBUTE_JINJA2_TEMPLATE_NAME, template_name) return wrapped(*args, **kwargs) @_with_tracer_wrapper def _wrap_compile(tracer, wrapped, _, args, kwargs): - template_name = ( - args[1] if len(args) > 1 else kwargs.get("name", DEFAULT_TEMPLATE_NAME) - ) - attributes = {ATTRIBUTE_JINJA2_TEMPLATE_NAME: template_name} with tracer.start_as_current_span( - "jinja2.compile", kind=SpanKind.INTERNAL, attributes=attributes - ): + "jinja2.compile", kind=SpanKind.INTERNAL, + ) as span: + if span.is_recording(): + template_name = ( + args[1] + if len(args) > 1 + else kwargs.get("name", DEFAULT_TEMPLATE_NAME) + ) + span.set_attribute(ATTRIBUTE_JINJA2_TEMPLATE_NAME, template_name) return wrapped(*args, **kwargs) @_with_tracer_wrapper def _wrap_load_template(tracer, wrapped, _, args, kwargs): - template_name = kwargs.get("name", args[0]) - attributes = {ATTRIBUTE_JINJA2_TEMPLATE_NAME: template_name} with tracer.start_as_current_span( - "jinja2.load", kind=SpanKind.INTERNAL, attributes=attributes + "jinja2.load", kind=SpanKind.INTERNAL, ) as span: + if span.is_recording(): + template_name = kwargs.get("name", args[0]) + span.set_attribute(ATTRIBUTE_JINJA2_TEMPLATE_NAME, template_name) template = None try: template = wrapped(*args, **kwargs) return template finally: - if template: + if template and span.is_recording(): span.set_attribute( ATTRIBUTE_JINJA2_TEMPLATE_PATH, template.filename ) diff --git a/instrumentation/opentelemetry-instrumentation-jinja2/tests/test_jinja2.py b/instrumentation/opentelemetry-instrumentation-jinja2/tests/test_jinja2.py index 717431abbb9..5de1d598cbc 100644 --- a/instrumentation/opentelemetry-instrumentation-jinja2/tests/test_jinja2.py +++ b/instrumentation/opentelemetry-instrumentation-jinja2/tests/test_jinja2.py @@ -13,6 +13,7 @@ # limitations under the License. import os +from unittest import mock import jinja2 @@ -53,6 +54,21 @@ def test_render_inline_template_with_root(self): self.assertIs(template.parent, root.get_span_context()) self.assertIsNone(root.parent) + def test_render_not_recording(self): + mock_tracer = mock.Mock() + mock_span = mock.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 mock.patch("opentelemetry.trace.get_tracer") as tracer: + tracer.return_value = mock_tracer + jinja2.environment.Template("Hello {{name}}!") + 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_render_inline_template(self): template = jinja2.environment.Template("Hello {{name}}!") self.assertEqual(template.render(name="Jinja"), "Hello Jinja!") diff --git a/instrumentation/opentelemetry-instrumentation-pymemcache/src/opentelemetry/instrumentation/pymemcache/__init__.py b/instrumentation/opentelemetry-instrumentation-pymemcache/src/opentelemetry/instrumentation/pymemcache/__init__.py index 46b188a3dfa..a91d3b525a3 100644 --- a/instrumentation/opentelemetry-instrumentation-pymemcache/src/opentelemetry/instrumentation/pymemcache/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-pymemcache/src/opentelemetry/instrumentation/pymemcache/__init__.py @@ -118,15 +118,16 @@ def _wrap_cmd(tracer, cmd, wrapped, instance, args, kwargs): _CMD, kind=SpanKind.INTERNAL, attributes={} ) as span: try: - if not args: - vals = "" - else: - vals = _get_query_string(args[0]) + if span.is_recording(): + if not args: + vals = "" + else: + vals = _get_query_string(args[0]) - query = "{}{}{}".format(cmd, " " if vals else "", vals) - span.set_attribute(_RAWCMD, query) + query = "{}{}{}".format(cmd, " " if vals else "", vals) + span.set_attribute(_RAWCMD, query) - _set_connection_attributes(span, instance) + _set_connection_attributes(span, instance) except Exception as ex: # pylint: disable=broad-except logger.warning( "Failed to set attributes for pymemcache span %s", str(ex) diff --git a/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py b/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py index 5357be6d0fc..f6ae8b321d0 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py @@ -226,9 +226,12 @@ def _start_span(tracer, handler, start_time) -> _TraceContext: span = tracer.start_span( _get_operation_name(handler, handler.request), kind=trace.SpanKind.SERVER, - attributes=_get_attributes_from_request(handler.request), start_time=start_time, ) + if span.is_recording(): + attributes = _get_attributes_from_request(handler.request) + for key, value in attributes.items(): + span.set_attribute(key, value) activation = tracer.use_span(span, end_on_exit=True) activation.__enter__() @@ -260,15 +263,16 @@ def _finish_span(tracer, handler, error=None): if not ctx: return - if reason: - ctx.span.set_attribute("http.status_text", reason) - ctx.span.set_attribute("http.status_code", status_code) - ctx.span.set_status( - Status( - canonical_code=http_status_to_canonical_code(status_code), - description=reason, + if ctx.span.is_recording(): + if reason: + ctx.span.set_attribute("http.status_text", reason) + ctx.span.set_attribute("http.status_code", status_code) + ctx.span.set_status( + Status( + canonical_code=http_status_to_canonical_code(status_code), + description=reason, + ) ) - ) ctx.activation.__exit__(*finish_args) context.detach(ctx.token)