Skip to content

Use is_recording flag in requests instrumentation #1087

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

Merged
merged 16 commits into from
Sep 14, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@
# pylint: disable=unused-argument
def _instrument(tracer_provider=None, span_callback=None):
"""Enables tracing of all requests calls that go through
:code:`requests.session.Session.request` (this includes
:code:`requests.get`, etc.)."""
:code:`requests.session.Session.request` (this includes
:code:`requests.get`, etc.)."""

# Since
# https://github.com/psf/requests/commit/d72d1162142d1bf8b1b5711c664fbbd674f349d1
Expand Down Expand Up @@ -121,9 +121,10 @@ def _instrumented_requests_call(
with get_tracer(
__name__, __version__, tracer_provider
).start_as_current_span(span_name, kind=SpanKind.CLIENT) as span:
span.set_attribute("component", "http")
span.set_attribute("http.method", method.upper())
span.set_attribute("http.url", url)
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)
Expand All @@ -139,13 +140,13 @@ def _instrumented_requests_call(
finally:
context.detach(token)

if exception is not None:
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 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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,28 @@ def test_suppress_instrumentation(self):

self.assert_span(num_spans=0)

def test_not_recording(self):
with mock.patch(
"opentelemetry.trace.INVALID_SPAN", autospec=True
) as mock_span:
RequestsInstrumentor().uninstrument()
# original_tracer_provider returns a default tracer provider, which
# in turn will return an INVALID_SPAN, which is always not recording
RequestsInstrumentor().instrument(
tracer_provider=self.original_tracer_provider
)
mock_span.is_recording.return_value = False
result = self.perform_request(self.URL)
self.assertEqual(result.text, "Hello!")
self.assert_span(None, 0)
with self.assertRaises(AttributeError):
# pylint: disable=pointless-statement
mock_span.attributes
Copy link
Member

Choose a reason for hiding this comment

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

I think the AttributeError is only raised because of autospec=True?

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_distributed_context(self):
previous_propagator = propagators.get_global_textmap()
try:
Expand Down
1 change: 1 addition & 0 deletions tests/util/setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ package_dir=
packages=find_namespace:
install_requires =
opentelemetry-api
opentelemetry-sdk

[options.extras_require]
test = flask~=1.0
Expand Down