chore(tracing): simplify Span tag/metric API typing #14943
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
brettlangdon
changed the title
brettlangdon
changed the title
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 209 ± 2 ms. The average import time from base is: 205 ± 2 ms. The import time difference between this PR and base is: 4.44 ± 0.08 ms. Import time breakdownThe following import paths have appeared:
|
mabdinur
approved these changes
Oct 20, 2025
| return self._meta_struct.get(key, None) | ||
|
|
||
| def set_tag_str(self, key: _TagNameType, value: Text) -> None: | ||
| def set_tag_str(self, key: str, value: str) -> None: |
There was a problem hiding this comment.
Should we deprecate/internalize this method? Ideally we should expose one public API for setting tags. If the overhead of handling sampling and service naming logic is significant we can move this logic out of set tags in v5.0
There was a problem hiding this comment.
I have another PR to deprecate and internalize it. With the changes to set_tag we might be able to just get rid of it entirely.
| # Ensure we do not have the same key in both meta and metrics | ||
| self._metrics.pop(key, None) | ||
|
|
||
| def set_struct_tag(self, key: str, value: Dict[str, Any]) -> None: |
There was a problem hiding this comment.
Should this method be public or is this functionality internal to the ddtrace library
There was a problem hiding this comment.
there is another PR to deprecate and internalize this
| return | ||
| elif key == SERVICE_KEY: | ||
| self.service = value | ||
| elif key == SERVICE_VERSION_KEY: |
There was a problem hiding this comment.
Can we handle these tags in a trace processor or in Logs backend? It seems unnecessary to do this check every time set_tag is called.
There was a problem hiding this comment.
good question... probably, but this change is started to get bloated a bit anyways, so maybe better to handle as a follow up.
it is an internal implementation detail, so... 🤷🏻
@mabdinur yeah, this is one of the bigger hidden issues with this change, since if someone doesn't notice the type changes and was doing the other approach we could take is to get rid of then splitting into meta/metrics for v0.4/v0.5 is an internal implementation detail only. |
emmettbutler
approved these changes
Oct 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
4.0
This updates all tag and metric methods to explicitly be
dict[str, str]anddict[str, int float]only.This removes the
value: Anyfromset_tagwhich allows us to remove a lot of checks for things like if it is a numeric type or not.Testing
Risks
Additional Notes