opentelemetry-sdk: fixup some pyright warnings#4749
opentelemetry-sdk: fixup some pyright warnings#4749xrmx wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
|
|
||
| def on_end(self, span: ReadableSpan) -> None: | ||
| if not span.context.trace_flags.sampled: | ||
| if span.context and not span.context.trace_flags.sampled: |
There was a problem hiding this comment.
Or the typing is wrong and context is not optional. Span has the same issue.
There was a problem hiding this comment.
I'm not sure the best way to go about this. ReadableSpan.context was made optional 2 years ago in #3528. I'm not sure why by looking at the PR and no linked issue. For Span.context it's not optional. Spec for Span Creation says api must accept Context, but I'm not sure if that means Context must not be null. If we changed ReadableSpan.context back to being mandatory that might break things.
There was a problem hiding this comment.
Do we need to fix it for this PR? The change LGTM
If we changed ReadableSpan.context back to being mandatory that might break things.
Since we have this warning
I think it would be OK to make it non-optional if you're able to guarantee it's always passed in. People shouldn't be constructing ReadableSpan, although they might do it in practice for testing.
|
|
||
| def on_end(self, span: ReadableSpan) -> None: | ||
| if not span.context.trace_flags.sampled: | ||
| if span.context and not span.context.trace_flags.sampled: |
There was a problem hiding this comment.
Do we need to fix it for this PR? The change LGTM
If we changed ReadableSpan.context back to being mandatory that might break things.
Since we have this warning
I think it would be OK to make it non-optional if you're able to guarantee it's always passed in. People shouldn't be constructing ReadableSpan, although they might do it in practice for testing.
pyproject.toml
Outdated
| "opentelemetry-sdk/src/opentelemetry/sdk/error_handler", | ||
| "opentelemetry-sdk/src/opentelemetry/sdk/metrics", | ||
| "opentelemetry-sdk/src/opentelemetry/sdk/trace/export", | ||
| "opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py", |
There was a problem hiding this comment.
can we remove this or still have issues?
There was a problem hiding this comment.
This requires the same span ifdefery that has been added here
xrmx
force-pushed
the
moar-typechecking
branch
from
b504184 to
fd382c9
Compare
xrmx
force-pushed
the
moar-typechecking
branch
from
fd382c9 to
deea7d2
Compare
xrmx
moved this from Easy to review / merge / close
to Reviewed PRs that need fixes
in @xrmx's Python PR digest
4 participants
Description
And then remove a bunch of dirs from the exclude list. Not sure if the code changes in SimpleSpanProcessor requires some tests with a span without context.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Contrib Repo Change?
Checklist: