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

Replaced Tracer.use_span() with opentelemetry.trace.use_span() #364

Merged
merged 1 commit into from
Mar 8, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ on:
- 'release/*'
pull_request:
env:
CORE_REPO_SHA: 9bf28fb451a85fd9e9a4f2276c3eebd484e55d02
CORE_REPO_SHA: ddff32ac77e2e22b3193b71f1e71a590a99d1eda

jobs:
build:
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased](https://github.com/open-telemetry/opentelemetry-python-contrib/compare/v0.18b0...HEAD)
- Updated instrumentations to use `opentelemetry.trace.use_span` instead of `Tracer.use_span()`
([#364](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/364))

### Changed
- Rename `IdsGenerator` to `IdGenerator`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
DatabaseApiIntegration,
)
from opentelemetry.trace import SpanKind
from opentelemetry.trace.status import Status, StatusCode


# pylint: disable=abstract-method
Expand Down Expand Up @@ -117,13 +116,7 @@ async def traced_execution(
name, kind=SpanKind.CLIENT
) as span:
self._populate_span(span, cursor, *args)
try:
result = await query_method(*args, **kwargs)
return result
except Exception as ex: # pylint: disable=broad-except
if span.is_recording():
span.set_status(Status(StatusCode.ERROR, str(ex)))
raise ex
return await query_method(*args, **kwargs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context manager above (start_as_current_span) handles exceptions and records them as status already so this was completely unnecessary/redundant. Some similar changes below.



def get_traced_cursor_proxy(cursor, db_api_integration, *args, **kwargs):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,6 @@ def test_span_not_recording(self):
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__ = True
db_integration = AiopgIntegration(
mock_tracer, "testcomponent", connection_attributes
)
Expand Down Expand Up @@ -322,7 +320,7 @@ def test_span_failed(self):
self.assertIs(
span.status.status_code, trace_api.status.StatusCode.ERROR
)
self.assertEqual(span.status.description, "Test Exception")
self.assertEqual(span.status.description, "Exception: Test Exception")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instrumentation was duplicating start_as_current_span's exception handling and not formatting the description properly. After removing the duplicate code, had to update the assertion to expect description formatted by the context manager. Similar changes below.


def test_executemany(self):
db_integration = AiopgIntegration(self.tracer, "testcomponent")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ def test_not_recording(self):
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__ = True
with patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
ec2 = boto.ec2.connect_to_region("us-west-2")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ def test_not_recording(self):
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__ = True
with patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
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 @@ -137,8 +137,8 @@ def _trace_prerun(self, *args, **kwargs):
operation_name, context=tracectx, kind=trace.SpanKind.CONSUMER
)

activation = self._tracer.use_span(span, end_on_exit=True)
activation.__enter__()
activation = trace.use_span(span, end_on_exit=True)
activation.__enter__() # pylint: disable=E1101
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pylint has false positives when using a generator as context manager so ignoring rule.

utils.attach_span(task, task_id, (span, activation))

@staticmethod
Expand Down Expand Up @@ -186,8 +186,9 @@ def _trace_before_publish(self, *args, **kwargs):
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__()
activation = trace.use_span(span, end_on_exit=True)
activation.__enter__() # pylint: disable=E1101

utils.attach_span(task, task_id, (span, activation), is_publish=True)

headers = kwargs.get("headers")
Expand All @@ -208,7 +209,7 @@ def _trace_after_publish(*args, **kwargs):
logger.warning("no existing span found for task_id=%s", task_id)
return

activation.__exit__(None, None, None)
activation.__exit__(None, None, None) # pylint: disable=E1101
utils.detach_span(task, task_id, is_publish=True)

@staticmethod
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,13 +371,7 @@ def traced_execution(
name, kind=SpanKind.CLIENT
) as span:
self._populate_span(span, cursor, *args)
try:
result = query_method(*args, **kwargs)
return result
except Exception as ex: # pylint: disable=broad-except
if span.is_recording():
span.set_status(Status(StatusCode.ERROR, str(ex)))
raise ex
return query_method(*args, **kwargs)


def get_traced_cursor_proxy(cursor, db_api_integration, *args, **kwargs):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@ def test_span_not_recording(self):
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__ = True
db_integration = dbapi.DatabaseApiIntegration(
mock_tracer, "testcomponent", connection_attributes
)
Expand Down Expand Up @@ -179,7 +177,7 @@ def test_span_failed(self):
self.assertIs(
span.status.status_code, trace_api.status.StatusCode.ERROR
)
self.assertEqual(span.status.description, "Test Exception")
self.assertEqual(span.status.description, "Exception: Test Exception")

def test_executemany(self):
db_integration = dbapi.DatabaseApiIntegration(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
collect_request_attributes,
)
from opentelemetry.propagate import extract
from opentelemetry.trace import SpanKind, get_tracer
from opentelemetry.trace import SpanKind, get_tracer, use_span
from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs

try:
Expand Down Expand Up @@ -118,8 +118,8 @@ def process_request(self, request):
for key, value in attributes.items():
span.set_attribute(key, value)

activation = tracer.use_span(span, end_on_exit=True)
activation.__enter__()
activation = use_span(span, end_on_exit=True)
activation.__enter__() # pylint: disable=E1101

request.META[self._environ_activation_key] = activation
request.META[self._environ_span_key] = span
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@ def test_not_recording(self):
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__ = True
with patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
Client().get("/traced/")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,19 +136,15 @@ def wrapper(wrapped, _, args, kwargs):
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) and span.is_recording():
for member in _ATTRIBUTES_FROM_RESULT:
if member in rv:
span.set_attribute(
"elasticsearch.{0}".format(member),
str(rv[member]),
)
return rv
except Exception as ex: # pylint: disable=broad-except
if span.is_recording():
span.set_status(Status(StatusCode.ERROR, str(ex)))
raise ex

rv = wrapped(*args, **kwargs)
if isinstance(rv, dict) and span.is_recording():
for member in _ATTRIBUTES_FROM_RESULT:
if member in rv:
span.set_attribute(
"elasticsearch.{0}".format(member),
str(rv[member]),
)
return rv

return wrapper
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from elasticsearch_dsl import Search

import opentelemetry.instrumentation.elasticsearch
from opentelemetry import trace
from opentelemetry.instrumentation.elasticsearch import (
ElasticsearchInstrumentor,
)
Expand Down Expand Up @@ -94,8 +95,6 @@ def test_span_not_recording(self, request_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()
Expand Down Expand Up @@ -174,7 +173,9 @@ def _test_trace_error(self, code, exc):
span = spans[0]
self.assertFalse(span.status.is_ok)
self.assertEqual(span.status.status_code, code)
self.assertEqual(span.status.description, str(exc))
self.assertEqual(
span.status.description, "{}: {}".format(type(exc).__name__, exc)
)

def test_parent(self, request_mock):
request_mock.return_value = (1, {}, {})
Expand All @@ -201,7 +202,7 @@ def test_multithread(self, request_mock):
# 2. Trace something from thread-2, make thread-1 join before finishing.
# 3. Check the spans got different parents, and are in the expected order.
def target1(parent_span):
with self.tracer.use_span(parent_span):
with trace.use_span(parent_span):
es.get(index="test-index", doc_type="tweet", id=1)
ev.set()
ev.wait()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

def __call__(self, env, start_response):
# pylint: disable=E1101
if _excluded_urls.url_disabled(env.get("PATH_INFO", "/")):
return super().__call__(env, start_response)

Expand All @@ -120,7 +121,7 @@ def __call__(self, env, start_response):
for key, value in attributes.items():
span.set_attribute(key, value)

activation = self._tracer.use_span(span, end_on_exit=True)
activation = trace.use_span(span, end_on_exit=True)
activation.__enter__()
env[_ENVIRON_SPAN_KEY] = span
env[_ENVIRON_ACTIVATION_KEY] = activation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,6 @@ def test_traced_not_recording(self):
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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ def _before_request():
for key, value in attributes.items():
span.set_attribute(key, value)

activation = tracer.use_span(span, end_on_exit=True)
activation.__enter__()
activation = trace.use_span(span, end_on_exit=True)
activation.__enter__() # pylint: disable=E1101
flask_request_environ[_ENVIRON_ACTIVATION_KEY] = activation
flask_request_environ[_ENVIRON_SPAN_KEY] = span
flask_request_environ[_ENVIRON_TOKEN] = token
Expand All @@ -148,6 +148,7 @@ def _before_request():


def _teardown_request(exc):
# pylint: disable=E1101
if _excluded_urls.url_disabled(flask.request.url):
return

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,6 @@ def test_not_recording(self):
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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,9 @@ def _set_remote_context(self, servicer_context):
else:
yield

def _start_span(self, handler_call_details, context):
def _start_span(
self, handler_call_details, context, set_status_on_exception=False
):

# standard attributes
attributes = {
Expand Down Expand Up @@ -234,6 +236,7 @@ def _start_span(self, handler_call_details, context):
name=handler_call_details.method,
kind=trace.SpanKind.SERVER,
attributes=attributes,
set_status_on_exception=set_status_on_exception,
)

def intercept_service(self, continuation, handler_call_details):
Expand All @@ -251,7 +254,9 @@ def telemetry_interceptor(request_or_iterator, context):

with self._set_remote_context(context):
with self._start_span(
handler_call_details, context
handler_call_details,
context,
set_status_on_exception=False,
) as span:
# wrap the context
context = _OpenTelemetryServicerContext(context, span)
Expand Down Expand Up @@ -283,7 +288,9 @@ def _intercept_server_stream(
):

with self._set_remote_context(context):
with self._start_span(handler_call_details, context) as span:
with self._start_span(
handler_call_details, context, set_status_on_exception=False
) as span:
context = _OpenTelemetryServicerContext(context, span)

try:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ def test_render_not_recording(self):
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}}!")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,6 @@ def test_not_recording(self):
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__ = True
Psycopg2Instrumentor().instrument()
with mock.patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@ def test_set_not_recording(self):
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__ = True
with mock.patch("opentelemetry.trace.get_tracer") as tracer:
tracer.return_value = mock_tracer
client = self.make_client([b"STORED\r\n"])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@ def test_not_recording(self):
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__ = True
mock_event = MockEvent({})
command_tracer = CommandTracer(mock_tracer)
command_tracer.started(event=mock_event)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ def _before_traversal(event):
for key, value in attributes.items():
span.set_attribute(key, value)

activation = tracer.use_span(span, end_on_exit=True)
activation.__enter__()
activation = trace.use_span(span, end_on_exit=True)
activation.__enter__() # pylint: disable=E1101
request_environ[_ENVIRON_ACTIVATION_KEY] = activation
request_environ[_ENVIRON_SPAN_KEY] = span
request_environ[_ENVIRON_TOKEN] = token
Expand All @@ -105,6 +105,7 @@ def disabled_tween(request):

# make a request tracing function
def trace_tween(request):
# pylint: disable=E1101
if _excluded_urls.url_disabled(request.url):
request.environ[_ENVIRON_ENABLED_KEY] = False
# short-circuit when we don't want to trace anything
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ def test_not_recording(self):
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__ = True
with patch("opentelemetry.trace.get_tracer"):
self.client.get("/hello/123")
span_list = self.memory_exporter.get_finished_spans()
Expand Down
Loading