Zipkin: Update span boolean attribute conversion #1509
Conversation
srikanthccv
requested review from
owais and
hectorhdzg
and removed request for
a team
| @@ -407,7 +411,7 @@ def _extract_tag_value_string_from_sequence(self, sequence: Sequence): | |||
| return json.dumps(tag_value_elements, separators=(",", ":")) | |||
|
|
|||
| def _extract_tags_from_span(self, span: Span): | |||
| tags = self._extract_tags_from_dict(getattr(span, "attributes", None)) | |||
| tags = self._extract_tags_from_dict(span.attributes) | |||
There was a problem hiding this comment.
Was there a specific reason we were using getattr?
There was a problem hiding this comment.
May be because API Span does not have "attributes" attribute and it might be used as a noop span in some situations? That'd be my guess.
There was a problem hiding this comment.
I see. Better of just leave it then. @lonewolf3739
There was a problem hiding this comment.
I dig into to see if I could find some relevant comments when this was added. #707 (comment) this was the discussion that introduced this.
There was a problem hiding this comment.
I am wondering If that is true we should be doing that all other attributes such as events, status, timestamps, kind and instrumentation_info etc.. and that should be done for not just zipkin but all exporters?
There was a problem hiding this comment.
Does SpanProcessor not take care of discarding those DefaultSpans?
There was a problem hiding this comment.
@toumorokoshi
Does this use case still apply?
There was a problem hiding this comment.
I don't think this would happen (we check for DefaultSpan in the span processor and it should never reach the exporter).
Fixes #1508