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.

Sign up for GitHub

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

Handle null statuses in http_status_to_status_code #823

Merged
merged 5 commits into from
Feb 2, 2022
Merged

Handle null statuses in http_status_to_status_code #823

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0147227
handle null statuses. fixes #820
gregbuehler Dec 3, 2021
365dc4f
#823 check for type instead of value, changelog
gregbuehler Dec 6, 2021
7debe40
fix whitespace issue in urllib
gregbuehler Jan 31, 2022
8aaa603
whitespace fixes
gregbuehler Feb 1, 2022
5ab080c
Merge branch 'main' into urllib-none-status
lzchen Feb 2, 2022
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 opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ def http_status_to_status_code(
status (int): HTTP status code
"""
# See: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#status
if status is None:
return StatusCode.UNSET
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about encoding knowledge about a special case in this function. IMO we should either have a isinstance(status, int) check or wrap in a try/except block to cover all cases of invalid input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

Would it also make sense to also try to coerce status to an integer in case the status was passed as a string representation while we're looking at this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about that. We don't necessarily want to work with a wide range of inputs. We just don't ever want to crash. I think the caller should be responsible for converting types.

Copy link
Member

Choose a reason for hiding this comment

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

Is this something common with other http client libraries as well? I personally prefer not to have the input sanitisation part here. And the caller would have the more context when the status is unusual and it can decide to what to do instead of we returning the UNSET here for all such scenarios.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should just include the check only for this specific instrumentation.

if status < 100:
return StatusCode.ERROR
if status <= 299:
Expand Down
8 changes: 8 additions & 0 deletions opentelemetry-instrumentation/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ def test_http_status_to_status_code(self):
actual = http_status_to_status_code(int(status_code))
self.assertEqual(actual, expected, status_code)

def test_http_status_to_status_code_none(self):
for status_code, expected in (
(None, StatusCode.UNSET),
):
with self.subTest(status_code=status_code):
actual = http_status_to_status_code(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),
Expand Down