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

Don't set STATUS on SpanKind SERVER for 4XX status #776

Merged
merged 6 commits into from
Oct 26, 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: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#774](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/774))
- `opentelemetry-instrumentation-django` Fixed carrier usage on ASGI requests.
([#767](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/767))
- Don't set Span Status on 4xx http status code for SpanKind.SERVER spans
([#776](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/776))

## [1.6.2-0.25b2](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.6.2-0.25b2) - 2021-10-19

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,9 @@ def set_status_code(span, status_code):
)
else:
span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
span.set_status(Status(http_status_to_status_code(status_code)))
span.set_status(
Status(http_status_to_status_code(status_code, server_span=True))
)


def get_default_span_details(scope: dict) -> Tuple[str, dict]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,9 @@ def process_response(
span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
span.set_status(
Status(
status_code=http_status_to_status_code(status_code),
status_code=http_status_to_status_code(
status_code, server_span=True
),
description=reason,
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,7 @@ def test_404(self):
self.assertEqual(len(spans), 1)
span = spans[0]
self.assertEqual(span.name, "HTTP GET")
self.assertEqual(span.status.status_code, StatusCode.ERROR)
self.assertEqual(
span.status.description, "NotFound",
)
self.assertEqual(span.status.status_code, StatusCode.UNSET)
self.assertSpanHasAttributes(
span,
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,9 @@ def _finish_span(tracer, handler, error=None):

if ctx.span.is_recording():
ctx.span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
otel_status_code = http_status_to_status_code(status_code)
otel_status_code = http_status_to_status_code(
status_code, server_span=True
)
otel_status_description = None
if otel_status_code is StatusCode.ERROR:
otel_status_description = reason
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,9 @@ def add_response_attributes(
)
else:
span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
span.set_status(Status(http_status_to_status_code(status_code)))
span.set_status(
Status(http_status_to_status_code(status_code, server_span=True))
)


def get_default_span_name(environ):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def extract_attributes_from_object(


def http_status_to_status_code(
status: int, allow_redirect: bool = True
status: int, allow_redirect: bool = True, server_span: bool = False,
) -> StatusCode:
"""Converts an HTTP status code to an OpenTelemetry canonical status code

Expand All @@ -50,6 +50,8 @@ def http_status_to_status_code(
return StatusCode.UNSET
if status <= 399 and allow_redirect:
return StatusCode.UNSET
if status <= 499 and server_span:
return StatusCode.UNSET
return StatusCode.ERROR


Expand Down
38 changes: 38 additions & 0 deletions opentelemetry-instrumentation/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,41 @@ def test_http_status_to_status_code(self):
with self.subTest(status_code=status_code):
actual = http_status_to_status_code(int(status_code))
self.assertEqual(actual, expected, status_code)

def test_http_status_to_status_code_redirect(self):
for status_code, expected in (
(HTTPStatus.MULTIPLE_CHOICES, StatusCode.ERROR),
(HTTPStatus.MOVED_PERMANENTLY, StatusCode.ERROR),
(HTTPStatus.TEMPORARY_REDIRECT, StatusCode.ERROR),
(HTTPStatus.PERMANENT_REDIRECT, StatusCode.ERROR),
):
with self.subTest(status_code=status_code):
actual = http_status_to_status_code(
int(status_code), allow_redirect=False
)
self.assertEqual(actual, expected, status_code)

def test_http_status_to_status_code_server(self):
for status_code, expected in (
(HTTPStatus.OK, StatusCode.UNSET),
(HTTPStatus.ACCEPTED, StatusCode.UNSET),
(HTTPStatus.IM_USED, StatusCode.UNSET),
(HTTPStatus.MULTIPLE_CHOICES, StatusCode.UNSET),
(HTTPStatus.BAD_REQUEST, StatusCode.UNSET),
(HTTPStatus.UNAUTHORIZED, StatusCode.UNSET),
(HTTPStatus.FORBIDDEN, StatusCode.UNSET),
(HTTPStatus.NOT_FOUND, StatusCode.UNSET),
(HTTPStatus.UNPROCESSABLE_ENTITY, StatusCode.UNSET,),
(HTTPStatus.TOO_MANY_REQUESTS, StatusCode.UNSET,),
(HTTPStatus.NOT_IMPLEMENTED, StatusCode.ERROR),
(HTTPStatus.SERVICE_UNAVAILABLE, StatusCode.ERROR),
(HTTPStatus.GATEWAY_TIMEOUT, StatusCode.ERROR,),
(HTTPStatus.HTTP_VERSION_NOT_SUPPORTED, StatusCode.ERROR,),
(600, StatusCode.ERROR),
(99, StatusCode.ERROR),
):
with self.subTest(status_code=status_code):
actual = http_status_to_status_code(
int(status_code), server_span=True
)
self.assertEqual(actual, expected, status_code)