Fix for byte type attributes crashing spans #775
Conversation
|
I'm not super familiar with how the spec works but it seems like bytes is just not allowed? I think this is the relevant section: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes
Does anyone know if this list of primitives is exhaustive? @sethmaxwl this might end up as a protobuf struct which would tell you whether or not bytes is just not allowed. If you can't send the raw bytes, decoding them as a string might not be the best thing to do in case the user passes bytes that aren't encoding a string at all. |
|
@aabmass raises a good point here. @sethmaxwl what's your use case / example for needing to consume a bytes object? Definitely room to amend the spec, but might be good to understand why it might make sense to do so. |
|
@toumorokoshi A use case is mentioned in #623 and shows setting |
| @@ -424,6 +424,8 @@ def set_attribute(self, key: str, value: types.AttributeValue) -> None: | |||
| # Freeze mutable sequences defensively | |||
| if isinstance(value, MutableSequence): | |||
| value = tuple(value) | |||
| if isinstance(value, bytes): | |||
| value = value.decode() | |||
There was a problem hiding this comment.
this could raise an exception due to mis-assuming the string-serializable encoding of the object.
It'd be great if we could choose an encoding that works universally, or maybe just ignore or skip issues: https://docs.python.org/3/library/codecs.html#codecs.decode
This commit ports the OpenTracing testbed[1] to check that the ot-shim is working as expected using different frameworks. Gevent doesn't support context vars yet[2], so those tests are not compatible with opentelemetry and were not ported. [1] https://github.com/opentracing/opentracing-python/tree/master/testbed [2] gevent/gevent#1407 Co-authored-by: Mauricio Vásquez <[email protected]> Co-authored-by: alrex <[email protected]>
Co-authored-by: Yusuke Tsutsumi <[email protected]>
|
LGTM, would be nice to add a test case too |
| @@ -487,6 +487,11 @@ def test_invalid_attribute_values(self): | |||
|
|
|||
There was a problem hiding this comment.
Would be nice to also add an invalid byte sequence here to assert it doesn't get added
There was a problem hiding this comment.
Would be good to check that it logs as well. Something like this:
…hmaxwl/opentelemetry-python into sdk-exporter-bytes-attribute
|
@sethmaxwl looks like travis is having issues with giving CI updates. Latest build on this PR is broken due to linting: https://travis-ci.org/github/open-telemetry/opentelemetry-python/jobs/695110178 Can you fix? and we can force merge if CI isn't updating appopriately. |
Just pushed a fix. |
This pull request addresses the behavior when setting byte type attributes in a span. When a byte type attribute is set, the exporter fails to convert the span to a JSON structure because the byte type is not JSON serializable, and the exporter crashes. This change decodes byte type attributes before adding them to the attributes dictionary.