-
Notifications
You must be signed in to change notification settings - Fork 533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set up typing for OTel #3168
Set up typing for OTel #3168
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INVALID_TRACE_ID
and INVALID_SPAN_ID
are integers while trace_id
and span_id
are strings, so this would never be True
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this new behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here: 4a4ca4a
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there no cast here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
cast
s), but all my other attempts at narrowing down types were unsuccessful.