Handle null statuses in http_status_to_status_code #823
Conversation
|
| @@ -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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Perhaps we should just include the check only for this specific instrumentation.
|
|
@gregbuehler |
|
|
@gregbuehler please fix the lint issues. Only your latest commit shows the "verified" label. I wonder if that is why the CLA check is not passing. Maybe merging them all in all in one single commit will make this test pass? |
gregbuehler
force-pushed
the
urllib-none-status
branch
from
5b88cd2 to
7debe40
Compare
Head branch was pushed to by a user without write access
gregbuehler
force-pushed
the
urllib-none-status
branch
from
e2506e8 to
8aaa603
Compare
|
Hello, Do you know when this fix will be included in a release ? Thanks |
Description
This handles None statuses passed to the http_status_to_status_code utility function. This treats None statuses as
StatusCode.UNSET.Fixes #820
Type of change
How Has This Been Tested?
This adds new unit tests.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.