fix: ensure status is always string#640
Conversation
During some testing, found that the pymongo instrumentation was setting the status description field as a dict which made the otcollector fail to process any spans. Added a test and wrapped the description field in a `str`.
|
IMHO you added a workaround for a bug in the pymongo instrumentation to the otcollector exporter. This workaround would need to be applied to every eporter, and every span processor that consumes the status directly in the end callback. I think the otcollector exporter is the wrong place to handle a non-string status. This should be fixed either in the Status constructor or the pymongo instrumentation (or both?). I think we should handle invalid types for status message consistently with invalid types for set_attribute. |
Fair enough, though my thinking here was that as the exporter, if the data type causes an exception, it might not be a bad idea to be a bit more defensive anyways. |
|
I second @Oberon00. I think the most straightforward solution here is to update the PyMongo integration to use a string in the description and update the |
| self.assertEqual(status.description, "unavailable") | ||
|
|
||
| def test_invalid_description(self): | ||
| status = Status(description={"test": "val"}) |
There was a problem hiding this comment.
Include a test checking that a warning is printed?
ext/opentelemetry-ext-otcollector/src/opentelemetry/ext/otcollector/trace_exporter/__init__.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>
mauriciovasquezbernal
left a comment
mauriciovasquezbernal
left a comment
There was a problem hiding this comment.
LGTM. Some code suggestions and a comment about the case included in the otcollector, but None of them is blocking.
| self.assertEqual( | ||
| output_spans[1].status.code, | ||
| trace_api.status.StatusCanonicalCode.INTERNAL.value, | ||
| ) | ||
| self.assertEqual(output_spans[1].status.message, "") |
There was a problem hiding this comment.
I think this particular test should not be included here as it's testing something implemented in a different place.
Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>
Ensuring that exporters can always expect status descriptions to be a string. This was implemented to be defensive against instrumentations such as PyMongo, which currently set status as a dict. Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io> Co-authored-by: Yusuke Tsutsumi <yusuke@tsutsumi.io>
Ensuring that exporters can always expect status descriptions to be a string. This was implemented to be defensive against instrumentations such as PyMongo, which currently set status as a dict. Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io> Co-authored-by: Yusuke Tsutsumi <yusuke@tsutsumi.io>
5 participants
During some testing, found that the pymongo instrumentation was setting the status description field as a dict which made the otcollector fail to process any spans.
Added a check for the description type to avoid the exporter being throwing an exception.
Additionally, added a check in the Status constructor to log and skip setting the description if it is not None and not a string.