fix(tracing): widen Span.set_tag value annotation to match implementation - #19814
fix(tracing): widen Span.set_tag value annotation to match implementation#19814ruidc wants to merge 1 commit into
Conversation
…tion The value parameter is annotated Optional[str] but the implementation accepts the historic domain: bools and bytes are stringified into tags, int/float values are stored as metrics. Widening to Any (the pre-4.0 annotation) lets five internal type-ignore comments be deleted. No runtime behaviour changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef88c14c2d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| """Set a tag key/value pair on the span. | ||
|
|
||
| Boolean and bytes values are stored as their string representation. | ||
| ``int`` and ``float`` values are stored as metrics (see ``set_metric``). |
There was a problem hiding this comment.
Qualify the numeric-to-metric guarantee
For integers outside the signed 64-bit range or non-finite floats, this new public documentation is incorrect: extract_attribute in src/native/span/span_data.rs stringifies overflowing integers and drops NaN/Infinity rather than storing them as metrics. Callers relying on this documented guarantee will therefore find a string tag or no value at all, so the sentence should state the supported numeric range and finite-value restriction. This docstring is customer-facing because Span is exposed through ddtrace.trace.
AGENTS.md reference: AGENTS.md:L25-L30
Useful? React with 👍 / 👎.
Fixes #19815
Description
Span.set_tagis annotated as acceptingvalue: Optional[str], but the implementation accepts andhandles the historic domain — booleans and bytes are stringified into tags, and
int/floatvaluesare stored as metrics. This PR widens the annotation back to
value: Any = None(the annotation ithad before v4.0), documents the value handling in the docstring, and deletes the five internal
# type: ignorecomments that existed only to silence the consequences of the too-narrow parameter.No runtime behaviour changes — the diff touches only the annotation, the docstring, comments, and a
release note.
Motivation
The
Optional[str]annotation makes type checkers reject valid application code:is correct at runtime — the body stringifies bools itself (the
isinstance(value, (bool, bytes))branch), and the emitted tag is byte-identical to passing
"True"— yet mypy reports:The implementation is the evidence that the annotation is too narrow. Five of its own
# type: ignorecomments exist purely because the parameter type doesn't cover what the body doeswith it:
int(value)fornet.TARGET_PORT,value = 1/set_metric(key, value)for_SPAN_MEASURED_KEY, and the two_set_attributecalls. All five become dead with the widenedannotation and are removed here (the repository's
warn_unused_ignores = trueenforces theirremoval).
The library's own test suite also pins the wider runtime contract:
tests/tracer/test_span_tags.py::test_tagsand::test_numeric_tags— ints and floats passed toset_tagare stored as metrics.tests/tracer/test_span_tags.py::test_set_tag_bool—set_tag("true", True)produces the tag"True".Relationship to the v4.0 typing change
I'm aware the narrowing was not accidental: it shipped in the v4.0 major-version PR (#14938,
commit
89d69bdfe) together with an explicit upgrade release note(
releasenotes/notes/explicit-span-tag-typing-99abb4d3ec065a55.yaml):This PR argues that annotation and implementation should agree: v4.0 narrowed the signature but
deliberately kept the lenient runtime handling (and the tests above that pin it), so the annotation
now rejects code the library explicitly supports. There is also direct precedent for resolving this
in favour of the implementation: #7182 (fixing #7175) made the same correction for
Span.set_tagsin 2023 — "type-checking would fail on valid application code due to a type hint … that was too
restrictive" — and chose
Any. If the v4.0 intent is instead that non-string values becomeunsupported, the five ignores and the bool/bytes/int handling would be the code to remove — a
behavioural break this PR deliberately does not make.
Considered alternative: a precise union
Optional[Union[str, bool, bytes, int, float]]was tried first, under the repository's owntoolchain (the pinned mypy 1.15.0 with
mypy.ini, which setswarn_unused_ignores = true). It isstrictly worse than
Any:value = 1), and[assignment]error atself.service = value—serviceisOptional[str]inddtrace/internal/native/_native.pyi— which would require a newsuppression.
value: Anymatches the pre-4.0 annotation and the #7182 precedent, removes all five suppressions,and adds none.
Testing
(
tests/tracer/test_span_tags.py).mypy --config-file mypy.ini(mypy 1.15.0, the pinned lint version): the reported error set isidentical before and after the change, and
ddtrace/_trace/span.pyreports zero diagnostics —including zero
unused-ignorewarnings after deleting the five comments.ruff check/ruff format --check(ruff 0.14.10, the pinned version) pass on the touched file.Risks
None at runtime — no executable code changes. The typing change is a widening, so no currently
type-checking caller breaks; code that was previously (incorrectly) rejected now checks.
Additional Notes
A release note is included
(
releasenotes/notes/widen-set-tag-value-typing-40a41e0a8804ec18.yaml) since this changes thepublic API's typing surface.
Span.set_tagsremainsdict[str, str]; widening it to match (as#7182 once did) would be a natural follow-up if this direction is accepted, but is deliberately out
of scope here.