Conversation
7a4df5c to
2e91fbb
Compare
| trace_id = format_trace_id(ctx.trace_id) | ||
| span_id = format_span_id(ctx.span_id) | ||
|
|
||
| if trace_id == INVALID_TRACE_ID or span_id == INVALID_SPAN_ID: |
There was a problem hiding this comment.
INVALID_TRACE_ID and INVALID_SPAN_ID are integers while trace_id and span_id are strings, so this would never be True.
antonpirker
left a comment
There was a problem hiding this comment.
I guess the otel_span.attributes.get() does not have a return type that "just works" for everything?
| if parent_context is None: | ||
| return | ||
|
|
There was a problem hiding this comment.
Why this new behavior?
There was a problem hiding this comment.
We need to do something about parent_context being possibly None because technically speaking, the next line, _get_trace_data, doesn't support parent_context being None.
I thought that without parent_context we can't really do anything after this, but double checking now I realize that's wrong, will fix!
| peer_name = otel_span.attributes.get(SpanAttributes.NET_PEER_NAME, None) | ||
| if peer_name: | ||
| description += " {}".format(peer_name) | ||
|
|
||
| target = otel_span.attributes.get(SpanAttributes.HTTP_TARGET, None) | ||
| if target: | ||
| description += " {}".format(target) |
There was a problem hiding this comment.
Why is there no cast here?
There was a problem hiding this comment.
Because mypy doesn't complain 🙃
Prolly because target is only used in the format string and whatever type it is, it'll have a __str__ defined, so no type issues there. Whereas for example for http_method above, it's __add__ed to a string which is only supported for strings, however mypy wasn't able to infer that it is in fact a string.
Ah sorry, missed this question last week.
Union[
str,
bool,
int,
float,
Sequence[str],
Sequence[bool],
Sequence[int],
Sequence[float],
]So you need to explicitly narrow the type down and tell mypy that a specific attribute can only be a subset of the |
This reverts commit 7c1685e.
This reverts commit 7c1685e.
TBH I'm not sure if this is the way to go (with all the
casts), but all my other attempts at narrowing down types were unsuccessful.