Skip to content
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

Change set_attributes to take a Mapping #4374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

camillol
Copy link

@camillol camillol commented Jan 2, 2025

Description

The attributes argument is currently specifies as a Dict[str, types.AttributeValue], which is invariant in its value type. As a result, you cannot pass a dictionary with a narrower value type, e.g. a dict[str, str]. But that should be allowed, since set_attributes simply adds all key-value pairs in its argument, and all such pairs are valid. Using Mapping takes care of this issue, since it's covariant in its value type.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Basic repro:
from opentelemetry import trace

span = trace.get_current_span()
attrs = {
    "key": "value",
}
span.set_attributes(attrs)

Check this with pyright. Without this change, you get an error like:

error: Argument of type "dict[str, str]" cannot be assigned to parameter "attributes" of type "Dict[str, AttributeValue]" in function "set_attributes"
  "dict[str, str]" is not assignable to "Dict[str, AttributeValue]"
    Type parameter "_VT@dict" is invariant, but "str" is not the same as "AttributeValue"
    Consider switching from "dict" to "Mapping" which is covariant in the value type (reportArgumentType)

With this change, it passes.

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

This currently specifies a Dict[str, types.AttributeValue], which is invariant in its value type. As a result, you cannot pass a dictionary with a narrower value type, e.g. a dict[str, str]. But this should be allowed, since set_attributes simply adds all key-value pairs in its argument, and all such pairs are valied. Using Mapping takes care of this issue, since it's covariant in its value type.
@camillol camillol requested a review from a team as a code owner January 2, 2025 06:11
@xrmx xrmx added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants