sdk: tighten ReadableSpan.attributes to non-Optional Mapping (#4569)

[WatchTree-19]

Description

ReadableSpan.attributes is annotated as types.Attributes, which resolves to Optional[Mapping[str, AttributeValue]]. The implementation always returns MappingProxyType(self._attributes or {}) - the or {} fallback ensures we never return None at runtime.

Pyright / Pylance flags Object of type "None" is not subscriptable on span.attributes["key"], forcing assert span.attributes is not None boilerplate at every call site.

This PR tightens the return annotation on ReadableSpan.attributes to the non-Optional Mapping[str, types.AttributeValue]. Implementation unchanged. Mapping is already imported in the file.

Scope:

• ReadableSpan.attributes only. Inherited by Span and _Span, no further changes needed.
• Event.attributes and Link.attributes return self._attributes directly (which can be None) and stay Optional.
• types.Attributes global alias is unchanged for the same reason.

Per the comment thread on #4569 from @exekis and @tekumara, the implementation has never returned None from ReadableSpan.attributes, so this is type-system tightening rather than a behaviour change. Technically a breaking change for any caller explicitly handling None on the return; in practice there shouldn't be any.

Fixes #4569

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

• patched file parses cleanly via python -m ast
• existing call sites are unchanged, only the return-type annotation is narrower
• have not run the project test suite locally; happy to chase a green CI here if anything trips

Does This PR Require a Contrib Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: WatchTree-19 / name: WatchTree-19 (11369be, 4188951)

[MikeGoldsmith]

Thanks @WatchTree-19 - left some feedback on things we need to tidy up

[MikeGoldsmith]

Looks like you got some other changelog entries in here - please can you clean this up?

[MikeGoldsmith]

This is very verbose, can we tighten this up?

[MikeGoldsmith]

Do / should we update the API definition too?

[WatchTree-19]

thanks @MikeGoldsmith. pushed a follow-up addressing all three:

1. CHANGELOG.md: hadn't realised the repo had moved to towncrier since my first commit went up - sorry for the noise. reverted the CHANGELOG.md edit to upstream/main and added .changelog/5183.changed as a one-line fragment per CONTRIBUTING.md.

2. docstring: tightened to a single line.

@property
def attributes(self) -> Mapping[str, types.AttributeValue]:
    # `or {}` keeps the return non-None; see #4569.
    return MappingProxyType(self._attributes or {})

3. API definition: looked at opentelemetry-api/src/opentelemetry/trace/span.py. the abstract trace_api.Span class defines set_attributes and set_attribute but no attributes property - that's only on the SDK's ReadableSpan. so no parallel API change is needed; the narrower annotation lives entirely in the SDK layer. happy to add a @property to the abstract API with the same Mapping[str, types.AttributeValue] return type if you'd prefer the surface symmetrical, but that'd be an API addition rather than a fix.

let me know if there's anything else.

[emdneto]

Thanks for the PR!

Just a heads-up: we no longer update CHANGELOG.md directly. The changelog is now generated from changelog fragments using Towncrier.

Please add the appropriate changelog fragment for this change instead of editing CHANGELOG.md manually. You can find the instructions and expected format in CONTRIBUTING.md.

[WatchTree-19]

@emdneto already addressed in the follow-up - reverted the CHANGELOG.md edit and added .changelog/5183.changed per CONTRIBUTING.md. see the latest commit + comment above.

[WatchTree-19]

@MikeGoldsmith @emdneto gentle bump - rebased onto current main to clear the CHANGELOG.md conflict that came in with the towncrier migration. branch is now one commit (SDK annotation tightening + .changelog/5183.changed fragment), DCO signed. ready for another look when you have time.

[github-actions]

This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment.
If you're still working on this, please add a comment or push new commits.

[WatchTree-19]

still active. branch is rebased onto current main, single commit (SDK annotation
tightening + .changelog/5183.changed), DCO signed. awaiting review from
@MikeGoldsmith or @emdneto when there's a window. happy to address any
remaining concerns.

[github-actions]

This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment.
If you're still working on this, please add a comment or push new commits.

[WatchTree-19]

@MikeGoldsmith thanks - addressed both points:

• "tighten this up": removed the explanatory comment entirely. MappingProxyType(self._attributes or {}) already makes the non-None guarantee self-evident, and the rationale lives in the changelog fragment / commit message, so the diff is now just the annotation change.

• "update the API definition too?": nothing to update on the API side. ReadableSpan is SDK-only, and the API Span interface (opentelemetry-api) only exposes attribute setters, not a readable attributes getter, so there's no abstract to narrow. the shared types.Attributes alias stays Optional on purpose - it's used for attribute inputs across the codebase (EventBase.attributes, span-creation params, etc.) that legitimately accept None. so this change is deliberately scoped to the one ReadableSpan.attributes return, where the implementation guarantees a Mapping.

re-requesting review. branch is behind main but conflict-free - happy to update it if you'd rather it be current before merge.

[github-actions]

This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment.
If you're still working on this, please add a comment or push new commits.

[WatchTree-19]

Rebased onto current main (11369be1) — the branch was showing behind, it's now clean and still a single commit.

Where this actually stands, since it's been round the stale-bot loop a few times:

• @tekumara approved 10 days ago.
• The only thing formally blocking is the CHANGES_REQUESTED from 70 days ago. Both points in it were addressed — the explanatory comment was dropped entirely, so the diff is now purely the annotation change, and there's no parallel API change to make because ReadableSpan is SDK-only (the API Span exposes attribute setters, not a readable getter). types.Attributes deliberately stays Optional, since it's the input alias used where None is legitimate.

@MikeGoldsmith — could you either re-review or dismiss the stale review if you're happy? Genuinely no rush on my side, but the review can't clear itself and the bot keeps threatening to bin it.

And if on reflection this isn't a change the SDK wants — narrowing a public return annotation is a real (if small) compatibility surface — I'd rather close it than keep it cycling. Just say and I'll take care of it.

[github-actions]

This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment.
If you're still working on this, please add a comment or push new commits.

[WatchTree-19]

gentle nudge before the stale bot closes this. the change request predates the rework, i tightened the annotation and @tekumara approved on the current shape. @MikeGoldsmith would you re-review when you get a chance? happy to adjust if anything still reads wrong.

[opentelemetry-pr-dashboard]

Pull request dashboard status

Waiting on the author · refreshed 2026-08-22 21:24 UTC

Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):

• Inline threads: 1

Status above doesn't look right?

• Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
• Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
• Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

[opentelemetry-pr-dashboard]

Hi @WatchTree-19 — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.

• Replying is enough to hand it off — answer, explain why no change is needed, or ask a follow-up. The dashboard routes it onward once nothing on the list is waiting on you.
• To hand it back for any other reason, including the dashboard getting this wrong, comment /dashboard route:reviewers.

[WatchTree-19]

@MikeGoldsmith friendly ping on this one, I think all three points are covered now:

• Changelog: cleaned up, it's a single .changelog/5183.changed fragment for this change only.
• Verbosity: the diff is down to the one line on ReadableSpan.attributes.
• API definition: the API Span has no attributes property, only set_attributes, so there's nothing to tighten there. ReadableSpan is SDK-only.

Rebased on main and green, and @tekumara approved in July. Mind taking another look when you get a chance?