Skip to content

opentelemetry-sdk: fixup some pyright warnings #4749

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+10 −6
Open

opentelemetry-sdk: fixup some pyright warnings #4749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d62571
opentelemetry-sdk: fixup some pyright warnings
xrmx Sep 12, 2025
fd382c9
More trivial typing for sdk
xrmx Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ def on_start(
span: The :class:`opentelemetry.trace.Span` that just started.
parent_context: The parent context of the span that just started.
"""
...

def on_end(self, span: "ReadableSpan") -> None:
"""Called when a :class:`opentelemetry.trace.Span` is ended.
Expand All @@ -122,9 +123,11 @@ def on_end(self, span: "ReadableSpan") -> None:
Args:
span: The :class:`opentelemetry.trace.Span` that just ended.
"""
...

def shutdown(self) -> None:
"""Called when a :class:`opentelemetry.sdk.trace.TracerProvider` is shutdown."""
...

def force_flush(self, timeout_millis: int = 30000) -> bool:
"""Export all ended spans to the configured Exporter that have not yet
Expand All @@ -137,6 +140,7 @@ def force_flush(self, timeout_millis: int = 30000) -> bool:
Returns:
False if the timeout is exceeded, True otherwise.
"""
...


# Temporary fix until https://github.com/PyCQA/pylint/issues/4098 is resolved
Expand Down Expand Up @@ -273,7 +277,7 @@ def force_flush(self, timeout_millis: int = 30000) -> bool:
timeout, False otherwise.
"""
futures = []
for sp in self._span_processors: # type: SpanProcessor
for sp in self._span_processors:
future = self._executor.submit(sp.force_flush, timeout_millis)
futures.append(future)

Expand Down
7 changes: 5 additions & 2 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,21 @@ def export(
Returns:
The result of the export
"""
...

def shutdown(self) -> None:
"""Shuts down the exporter.

Called when the SDK is shut down.
"""
...

def force_flush(self, timeout_millis: int = 30000) -> bool:
"""Hint to ensure that the export of any spans the exporter has received
prior to the call to ForceFlush SHOULD be completed as soon as possible, preferably
before returning from this method.
"""
...


class SimpleSpanProcessor(SpanProcessor):
Expand All @@ -102,7 +105,7 @@ def on_start(
pass

def on_end(self, span: ReadableSpan) -> None:
if not span.context.trace_flags.sampled:
if span.context and not span.context.trace_flags.sampled:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or the typing is wrong and context is not optional. Span has the same issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

Lines 369 to 373 in 5c8269c

class ReadableSpan:
"""Provides read-only access to span attributes.
Users should NOT be creating these objects directly. `ReadableSpan`s are created as
a direct result from using the tracing pipeline via the `Tracer`.

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.

return
token = attach(set_value(_SUPPRESS_INSTRUMENTATION_KEY, True))
try:
Expand Down Expand Up @@ -188,7 +191,7 @@ def on_start(
pass

def on_end(self, span: ReadableSpan) -> None:
if not span.context.trace_flags.sampled:
if span.context and not span.context.trace_flags.sampled:
return
self._batch_processor.emit(span)

Expand Down
3 changes: 0 additions & 3 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,8 @@ exclude = [
"opentelemetry-sdk/tests",
"opentelemetry-sdk/src/opentelemetry/sdk/_events",
"opentelemetry-sdk/src/opentelemetry/sdk/_logs",
"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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this or still have issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires the same span ifdefery that has been added here

"opentelemetry-sdk/src/opentelemetry/sdk/util",
"opentelemetry-sdk/benchmarks",
]

Expand Down
Loading