Skip to content

Change status codes from grpc status codes, remove setting status in instrumentations except on ERROR #1282

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 25 commits into from
Oct 28, 2020
Merged
Show file tree
Hide file tree
Changes from 17 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
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import opentelemetry.trace as trace_api
from opentelemetry.sdk.trace import sampling
from opentelemetry.sdk.trace.export import SpanExporter, SpanExportResult
from opentelemetry.trace.status import StatusCanonicalCode

# pylint:disable=relative-beyond-top-level
from .constants import (
Expand Down Expand Up @@ -145,7 +144,7 @@ def _translate_to_datadog(self, spans):
datadog_span.start_ns = span.start_time
datadog_span.duration_ns = span.end_time - span.start_time

if span.status.canonical_code is not StatusCanonicalCode.OK:
if not span.status.is_ok:
datadog_span.error = 1
if span.status.description:
exc_type, exc_val = _get_exc_info(span)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
from opentelemetry.exporter.jaeger.gen.agent import Agent as agent
from opentelemetry.exporter.jaeger.gen.jaeger import Collector as jaeger
from opentelemetry.sdk.trace.export import Span, SpanExporter, SpanExportResult
from opentelemetry.trace.status import StatusCanonicalCode
from opentelemetry.trace.status import StatusCode

DEFAULT_AGENT_HOST_NAME = "localhost"
DEFAULT_AGENT_PORT = 6831
Expand Down Expand Up @@ -245,7 +245,7 @@ def _translate_to_jaeger(spans: Span):
)

# Ensure that if Status.Code is not OK, that we set the "error" tag on the Jaeger span.
if status.canonical_code is not StatusCanonicalCode.OK:
if not status.is_ok:
tags.append(_get_bool_tag("error", True))

refs = _extract_refs_from_span(span)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
from opentelemetry.sdk import trace
from opentelemetry.sdk.trace import Resource
from opentelemetry.sdk.util.instrumentation import InstrumentationInfo
from opentelemetry.trace.status import Status, StatusCanonicalCode
from opentelemetry.trace.status import Status, StatusCode


class TestJaegerSpanExporter(unittest.TestCase):
Expand Down Expand Up @@ -210,7 +210,7 @@ def test_translate_to_jaeger(self):
jaeger.Tag(
key="status.code",
vType=jaeger.TagType.LONG,
vLong=StatusCanonicalCode.OK.value,
vLong=StatusCode.UNSET.value,
),
jaeger.Tag(
key="status.message", vType=jaeger.TagType.STRING, vStr=None
Expand Down Expand Up @@ -249,7 +249,7 @@ def test_translate_to_jaeger(self):
attributes={"key_resource": "some_resource"}
)
otel_spans[0].set_status(
Status(StatusCanonicalCode.UNKNOWN, "Example description")
Status(StatusCode.ERROR, "Example description")
)
otel_spans[0].end(end_time=end_times[0])

Expand All @@ -259,6 +259,9 @@ def test_translate_to_jaeger(self):

otel_spans[2].start(start_time=start_times[2])
otel_spans[2].resource = Resource({})
otel_spans[2].set_status(
Status(StatusCode.OK, "Example description")
)
otel_spans[2].end(end_time=end_times[2])
otel_spans[2].instrumentation_info = InstrumentationInfo(
name="name", version="version"
Expand Down Expand Up @@ -304,7 +307,7 @@ def test_translate_to_jaeger(self):
jaeger.Tag(
key="status.code",
vType=jaeger.TagType.LONG,
vLong=StatusCanonicalCode.UNKNOWN.value,
vLong=StatusCode.ERROR.value,
),
jaeger.Tag(
key="status.message",
Expand Down Expand Up @@ -380,12 +383,12 @@ def test_translate_to_jaeger(self):
jaeger.Tag(
key="status.code",
vType=jaeger.TagType.LONG,
vLong=StatusCanonicalCode.OK.value,
vLong=StatusCode.OK.value,
),
jaeger.Tag(
key="status.message",
vType=jaeger.TagType.STRING,
vStr=None,
vStr="Example description",
),
jaeger.Tag(
key="span.kind",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,15 +148,15 @@ def test_translate_to_collector(self):
otel_spans[0].set_attribute("key_int", 333)
otel_spans[0].set_status(
trace_api.Status(
trace_api.status.StatusCanonicalCode.INTERNAL,
trace_api.status.StatusCode.OK,
"test description",
)
)
otel_spans[0].end(end_time=end_times[0])
otel_spans[1].start(start_time=start_times[1])
otel_spans[1].set_status(
trace_api.Status(
trace_api.status.StatusCanonicalCode.INTERNAL, {"test", "val"},
trace_api.status.StatusCode.ERROR, {"test", "val"},
)
)
otel_spans[1].end(end_time=end_times[1])
Expand Down Expand Up @@ -198,7 +198,7 @@ def test_translate_to_collector(self):
)
self.assertEqual(
output_spans[0].status.code,
trace_api.status.StatusCanonicalCode.INTERNAL.value,
trace_api.status.StatusCode.OK.value,
)
self.assertEqual(output_spans[0].status.message, "test description")
self.assertEqual(len(output_spans[0].tracestate.entries), 1)
Expand Down Expand Up @@ -270,7 +270,7 @@ def test_translate_to_collector(self):
)
self.assertEqual(
output_spans[1].status.code,
trace_api.status.StatusCanonicalCode.INTERNAL.value,
trace_api.status.StatusCode.ERROR.value,
)
self.assertEqual(
output_spans[2].links.link[0].type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from opentelemetry.proto.trace.v1.trace_pb2 import Status
from opentelemetry.sdk.trace import Span as SDKSpan
from opentelemetry.sdk.trace.export import SpanExporter, SpanExportResult
from opentelemetry.trace.status import StatusCode

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -159,8 +160,12 @@ def _translate_links(self, sdk_span: SDKSpan) -> None:

def _translate_status(self, sdk_span: SDKSpan) -> None:
if sdk_span.status is not None:
# TODO: Update this when the proto definitions are updated to include UNSET and ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO required for GA? If so, may be create an issue and link it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

proto_status_code = Status.STATUS_CODE_OK
if sdk_span.status.canonical_code is StatusCode.ERROR:
proto_status_code = Status.STATUS_CODE_UNKNOWN_ERROR
self._collector_span_kwargs["status"] = Status(
code=sdk_span.status.canonical_code.value,
code=proto_status_code,
message=sdk_span.status.description,
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from opentelemetry.sdk.trace.export import SpanExportResult
from opentelemetry.sdk.util.instrumentation import InstrumentationInfo
from opentelemetry.trace import TraceFlags
from opentelemetry.trace.status import Status, StatusCanonicalCode
from opentelemetry.trace.status import Status, StatusCode


class MockResponse:
Expand Down Expand Up @@ -179,7 +179,7 @@ def test_export(self):
otel_spans[0].set_attribute("key_string", "hello_world")
otel_spans[0].set_attribute("key_float", 111.22)
otel_spans[0].set_status(
Status(StatusCanonicalCode.UNKNOWN, "Example description")
Status(StatusCode.ERROR, "Example description")
)
otel_spans[0].end(end_time=end_times[0])

Expand Down Expand Up @@ -248,7 +248,7 @@ def test_export(self):
"kind": None,
"tags": {
"key_resource": "some_resource",
"otel.status_code": "0",
"otel.status_code": "1",
},
"annotations": None,
},
Expand All @@ -263,7 +263,7 @@ def test_export(self):
"tags": {
"key_string": "hello_world",
"key_resource": "some_resource",
"otel.status_code": "0",
"otel.status_code": "1",
},
"annotations": None,
},
Expand All @@ -278,7 +278,7 @@ def test_export(self):
"tags": {
"otel.instrumentation_library.name": "name",
"otel.instrumentation_library.version": "version",
"otel.status_code": "0",
"otel.status_code": "1",
},
"annotations": None,
},
Expand Down Expand Up @@ -356,7 +356,7 @@ def test_zero_padding(self):
"duration": duration // 10 ** 3,
"localEndpoint": local_endpoint,
"kind": None,
"tags": {"otel.status_code": "0"},
"tags": {"otel.status_code": "1"},
"annotations": None,
"debug": True,
"parentId": "0aaaaaaaaaaaaaaa",
Expand Down Expand Up @@ -401,7 +401,7 @@ def test_max_tag_length(self):
span.set_attribute("k1", "v" * 500)
span.set_attribute("k2", "v" * 50)
span.set_status(
Status(StatusCanonicalCode.UNKNOWN, "Example description")
Status(StatusCode.ERROR, "Example description")
)
span.end()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def strip_query_params(url: yarl.URL) -> str:
unwrap,
)
from opentelemetry.trace import SpanKind, TracerProvider, get_tracer
from opentelemetry.trace.status import Status, StatusCanonicalCode
from opentelemetry.trace.status import Status, StatusCode

_UrlFilterT = typing.Optional[typing.Callable[[str], str]]
_SpanNameT = typing.Optional[
Expand Down Expand Up @@ -219,16 +219,17 @@ async def on_request_exception(
params.exception,
(aiohttp.ServerTimeoutError, aiohttp.TooManyRedirects),
):
status = StatusCanonicalCode.DEADLINE_EXCEEDED
status = StatusCode.ERROR
# Assume any getaddrinfo error is a DNS failure.
elif isinstance(
params.exception, aiohttp.ClientConnectorError
) and isinstance(params.exception.os_error, socket.gaierror):
# DNS resolution failed
status = StatusCanonicalCode.UNKNOWN
status = StatusCode.ERROR
else:
status = StatusCanonicalCode.UNAVAILABLE
status = StatusCode.ERROR

# TODO: Remove setting status in instrumentation
trace_config_ctx.span.set_status(Status(status))
trace_config_ctx.span.record_exception(params.exception)
_end_trace(trace_config_ctx)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
AioHttpClientInstrumentor,
)
from opentelemetry.test.test_base import TestBase
from opentelemetry.trace.status import StatusCanonicalCode
from opentelemetry.trace.status import StatusCode


def run_with_test_server(
Expand Down Expand Up @@ -111,12 +111,12 @@ async def client_request(server: aiohttp.test_utils.TestServer):

def test_status_codes(self):
for status_code, span_status in (
(HTTPStatus.OK, StatusCanonicalCode.OK),
(HTTPStatus.TEMPORARY_REDIRECT, StatusCanonicalCode.OK),
(HTTPStatus.SERVICE_UNAVAILABLE, StatusCanonicalCode.UNAVAILABLE),
(HTTPStatus.OK, StatusCode.UNSET),
(HTTPStatus.TEMPORARY_REDIRECT, StatusCode.UNSET),
(HTTPStatus.SERVICE_UNAVAILABLE, StatusCode.ERROR),
(
HTTPStatus.GATEWAY_TIMEOUT,
StatusCanonicalCode.DEADLINE_EXCEEDED,
StatusCode.ERROR,
),
):
with self.subTest(status_code=status_code):
Expand Down Expand Up @@ -188,7 +188,7 @@ def test_span_name_option(self):
[
(
expected,
(StatusCanonicalCode.OK, None),
(StatusCode.UNSET, None),
{
"component": "http",
"http.method": method,
Expand Down Expand Up @@ -220,7 +220,7 @@ def strip_query_params(url: yarl.URL) -> str:
[
(
"HTTP GET",
(StatusCanonicalCode.OK, None),
(StatusCode.UNSET, None),
{
"component": "http",
"http.method": "GET",
Expand All @@ -238,8 +238,8 @@ def test_connection_errors(self):
trace_configs = [aiohttp_client.create_trace_config()]

for url, expected_status in (
("http://this-is-unknown.local/", StatusCanonicalCode.UNKNOWN),
("http://127.0.0.1:1/", StatusCanonicalCode.UNAVAILABLE),
("http://this-is-unknown.local/", StatusCode.ERROR),
("http://127.0.0.1:1/", StatusCode.ERROR),
):
with self.subTest(expected_status=expected_status):

Expand Down Expand Up @@ -286,7 +286,7 @@ async def request_handler(request):
[
(
"HTTP GET",
(StatusCanonicalCode.DEADLINE_EXCEEDED, None),
(StatusCode.ERROR, None),
{
"component": "http",
"http.method": "GET",
Expand Down Expand Up @@ -316,7 +316,7 @@ async def request_handler(request):
[
(
"HTTP GET",
(StatusCanonicalCode.DEADLINE_EXCEEDED, None),
(StatusCode.ERROR, None),
{
"component": "http",
"http.method": "GET",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
TracedCursor,
)
from opentelemetry.trace import SpanKind
from opentelemetry.trace.status import Status, StatusCanonicalCode
from opentelemetry.trace.status import Status, StatusCode


# pylint: disable=abstract-method
Expand Down Expand Up @@ -109,12 +109,13 @@ async def traced_execution(
try:
result = await query_method(*args, **kwargs)
if span.is_recording():
span.set_status(Status(StatusCanonicalCode.OK))
# TODO: Remove setting status in instrumentation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# TODO: Remove setting status in instrumentation
# TODO: Remove setting OK status in instrumentation

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's not exactly accurate. The TODO is removing any set_status in instrumentations that pertain to OK or UNSET. I should probably name it to remove setting non-ERROR status in instrumentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I made the changes and will be just removing the TODOs.

span.set_status(Status(StatusCode.UNSET))
return result
except Exception as ex: # pylint: disable=broad-except
if span.is_recording():
span.set_status(
Status(StatusCanonicalCode.UNKNOWN, str(ex))
Status(StatusCode.ERROR, str(ex))
)
raise ex

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ def test_span_succeeded(self):
self.assertEqual(span.attributes["net.peer.port"], 123)
self.assertIs(
span.status.canonical_code,
trace_api.status.StatusCanonicalCode.OK,
trace_api.status.StatusCode.UNSET,
)

def test_span_not_recording(self):
Expand Down Expand Up @@ -279,7 +279,7 @@ def test_span_failed(self):
self.assertEqual(span.attributes["db.statement"], "Test query")
self.assertIs(
span.status.canonical_code,
trace_api.status.StatusCanonicalCode.UNKNOWN,
trace_api.status.StatusCode.ERROR,
)
self.assertEqual(span.status.description, "Test Exception")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from opentelemetry import context, propagators, trace
from opentelemetry.instrumentation.asgi.version import __version__ # noqa
from opentelemetry.instrumentation.utils import http_status_to_canonical_code
from opentelemetry.trace.status import Status, StatusCanonicalCode
from opentelemetry.trace.status import Status, StatusCode


def get_header_from_scope(scope: dict, header_name: str) -> typing.List[str]:
Expand Down Expand Up @@ -91,14 +91,15 @@ def collect_request_attributes(scope):

def set_status_code(span, status_code):
"""Adds HTTP response attributes to span using the status_code argument."""
# TODO: Remove setting status in instrumentation
if not span.is_recording():
return
try:
status_code = int(status_code)
except ValueError:
span.set_status(
Status(
StatusCanonicalCode.UNKNOWN,
StatusCode.ERROR,
"Non-integer HTTP status: " + repr(status_code),
)
)
Expand Down
Loading