-
Notifications
You must be signed in to change notification settings - Fork 463
chore(tracing): simplify Span tag/metric API typing #14943
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
base: 4.0-breaking-changes
Are you sure you want to change the base?
Changes from 5 commits
b089f94
d72a632
92b8541
b199561
f725283
c66e659
19a56c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,9 +20,6 @@ | |
from ddtrace._trace._span_pointer import _SpanPointerDirection | ||
from ddtrace._trace.context import Context | ||
from ddtrace._trace.types import _AttributeValueType | ||
from ddtrace._trace.types import _MetaDictType | ||
from ddtrace._trace.types import _MetricDictType | ||
from ddtrace._trace.types import _TagNameType | ||
from ddtrace.constants import _SAMPLING_AGENT_DECISION | ||
from ddtrace.constants import _SAMPLING_LIMIT_DECISION | ||
from ddtrace.constants import _SAMPLING_RULE_DECISION | ||
|
@@ -37,14 +34,10 @@ | |
from ddtrace.constants import USER_KEEP | ||
from ddtrace.constants import USER_REJECT | ||
from ddtrace.constants import VERSION_KEY | ||
from ddtrace.ext import http | ||
from ddtrace.ext import net | ||
from ddtrace.internal import core | ||
from ddtrace.internal._rand import rand64bits as _rand64bits | ||
from ddtrace.internal._rand import rand128bits as _rand128bits | ||
from ddtrace.internal.compat import NumericType | ||
from ddtrace.internal.compat import ensure_text | ||
from ddtrace.internal.compat import is_integer | ||
from ddtrace.internal.constants import MAX_INT_64BITS as _MAX_INT_64BITS | ||
from ddtrace.internal.constants import MAX_UINT_64BITS as _MAX_UINT_64BITS | ||
from ddtrace.internal.constants import MIN_INT_64BITS as _MIN_INT_64BITS | ||
|
@@ -191,9 +184,9 @@ def __init__( | |
self.span_type = span_type | ||
self._span_api = span_api | ||
|
||
self._meta: _MetaDictType = {} | ||
self._meta: dict[str, str] = {} | ||
self.error = 0 | ||
self._metrics: _MetricDictType = {} | ||
self._metrics: dict[str, NumericType] = {} | ||
|
||
self._meta_struct: Dict[str, Dict[str, Any]] = {} | ||
|
||
|
@@ -335,51 +328,17 @@ def _set_sampling_decision_maker( | |
self.context._meta[SAMPLING_DECISION_TRACE_TAG_KEY] = value | ||
return value | ||
|
||
def set_tag(self, key: _TagNameType, value: Any = None) -> None: | ||
def set_tag(self, key: str, value: Optional[str] = None) -> None: | ||
"""Set a tag key/value pair on the span. | ||
|
||
Keys must be strings, values must be ``str``-able. | ||
Keys must be strings, values must be ``str``. | ||
|
||
:param key: Key to use for the tag | ||
:type key: str | ||
:type key: ``str`` | ||
:param value: Value to assign for the tag | ||
:type value: ``str``-able value | ||
:type value: ``str`` | `None`` | ||
""" | ||
|
||
if not isinstance(key, str): | ||
log.warning("Ignoring tag pair %s:%s. Key must be a string.", key, value) | ||
return | ||
|
||
# Special case, force `http.status_code` as a string | ||
# DEV: `http.status_code` *has* to be in `meta` for metrics | ||
# calculated in the trace agent | ||
if key == http.STATUS_CODE: | ||
value = str(value) | ||
|
||
# Determine once up front | ||
val_is_an_int = is_integer(value) | ||
|
||
# Explicitly try to convert expected integers to `int` | ||
# DEV: Some integrations parse these values from strings, but don't call `int(value)` themselves | ||
INT_TYPES = (net.TARGET_PORT,) | ||
if key in INT_TYPES and not val_is_an_int: | ||
try: | ||
value = int(value) | ||
val_is_an_int = True | ||
except (ValueError, TypeError): | ||
pass | ||
|
||
# Set integers that are less than equal to 2^53 as metrics | ||
if value is not None and val_is_an_int and abs(value) <= 2**53: | ||
self.set_metric(key, value) | ||
return | ||
|
||
# All floats should be set as a metric | ||
elif isinstance(value, float): | ||
self.set_metric(key, value) | ||
return | ||
|
||
elif key == MANUAL_KEEP_KEY: | ||
if key == MANUAL_KEEP_KEY: | ||
self._override_sampling_decision(USER_KEEP) | ||
return | ||
elif key == MANUAL_DROP_KEY: | ||
|
@@ -390,21 +349,17 @@ def set_tag(self, key: _TagNameType, value: Any = None) -> None: | |
elif key == SERVICE_VERSION_KEY: | ||
# Also set the `version` tag to the same value | ||
# DEV: Note that we do no return, we want to set both | ||
self.set_tag(VERSION_KEY, value) | ||
elif key == _SPAN_MEASURED_KEY: | ||
# Set `_dd.measured` tag as a metric | ||
# DEV: `set_metric` will ensure it is an integer 0 or 1 | ||
if value is None: | ||
value = 1 | ||
self.set_metric(key, value) | ||
return | ||
if value: | ||
self._meta[VERSION_KEY] = value | ||
else: | ||
self._meta.pop(VERSION_KEY, None) | ||
|
||
try: | ||
self._meta[key] = str(value) | ||
if key in self._metrics: | ||
del self._metrics[key] | ||
except Exception: | ||
log.warning("error setting tag %s, ignoring it", key, exc_info=True) | ||
if value is not None: | ||
self._meta[key] = value | ||
else: | ||
self._meta.pop(key, None) | ||
# 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this method be public or is this functionality internal to the ddtrace library There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is another PR to deprecate and internalize this |
||
""" | ||
|
@@ -417,35 +372,29 @@ def get_struct_tag(self, key: str) -> Optional[Dict[str, Any]]: | |
"""Return the given struct or None if it doesn't exist.""" | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have another PR to deprecate and internalize it. With the changes to |
||
"""Set a value for a tag. Values are coerced to unicode in Python 2 and | ||
str in Python 3, with decoding errors in conversion being replaced with | ||
U+FFFD. | ||
""" | ||
try: | ||
self._meta[key] = ensure_text(value, errors="replace") | ||
except Exception as e: | ||
if config._raise: | ||
raise e | ||
log.warning("Failed to set text tag '%s'", key, exc_info=True) | ||
self._meta[key] = value | ||
|
||
def get_tag(self, key: _TagNameType) -> Optional[Text]: | ||
def get_tag(self, key: str) -> Optional[str]: | ||
"""Return the given tag or None if it doesn't exist.""" | ||
return self._meta.get(key, None) | ||
|
||
def get_tags(self) -> _MetaDictType: | ||
def get_tags(self) -> dict[str, str]: | ||
"""Return all tags.""" | ||
return self._meta.copy() | ||
|
||
def set_tags(self, tags: Dict[_TagNameType, Any]) -> None: | ||
def set_tags(self, tags: dict[str, str]) -> None: | ||
"""Set a dictionary of tags on the given span. Keys and values | ||
must be strings (or stringable) | ||
""" | ||
if tags: | ||
for k, v in iter(tags.items()): | ||
self.set_tag(k, v) | ||
for k, v in tags.items(): | ||
self.set_tag(k, v) | ||
|
||
def set_metric(self, key: _TagNameType, value: NumericType) -> None: | ||
def set_metric(self, key: str, value: NumericType) -> None: | ||
"""This method sets a numeric tag value for the given key.""" | ||
# Enforce a specific constant for `_dd.measured` | ||
if key == _SPAN_MEASURED_KEY: | ||
|
@@ -455,35 +404,23 @@ def set_metric(self, key: _TagNameType, value: NumericType) -> None: | |
log.warning("failed to convert %r tag to an integer from %r", key, value) | ||
return | ||
|
||
# FIXME[matt] we could push this check to serialization time as well. | ||
# only permit types that are commonly serializable (don't use | ||
# isinstance so that we convert unserializable types like numpy | ||
# numbers) | ||
if not isinstance(value, (int, float)): | ||
try: | ||
value = float(value) | ||
except (ValueError, TypeError): | ||
log.debug("ignoring not number metric %s:%s", key, value) | ||
return | ||
|
||
# don't allow nan or inf | ||
if math.isnan(value) or math.isinf(value): | ||
log.debug("ignoring not real metric %s:%s", key, value) | ||
return | ||
|
||
if key in self._meta: | ||
del self._meta[key] | ||
self._metrics[key] = value | ||
# Ensure we do not have the same key in both meta and metrics | ||
self._meta.pop(key, None) | ||
|
||
def set_metrics(self, metrics: _MetricDictType) -> None: | ||
def set_metrics(self, metrics: dict[str, NumericType]) -> None: | ||
"""Set a dictionary of metrics on the given span. Keys must be | ||
must be strings (or stringable). Values must be numeric. | ||
""" | ||
if metrics: | ||
for k, v in metrics.items(): | ||
self.set_metric(k, v) | ||
for k, v in metrics.items(): | ||
self.set_metric(k, v) | ||
|
||
def get_metric(self, key: _TagNameType) -> Optional[NumericType]: | ||
def get_metric(self, key: str) -> Optional[NumericType]: | ||
"""Return the given metric or None if it doesn't exist.""" | ||
return self._metrics.get(key) | ||
|
||
|
@@ -496,7 +433,7 @@ def _add_on_finish_exception_callback(self, callback: Callable[["Span"], None]): | |
"""Add an errortracking related callback to the on_finish_callback array""" | ||
self._on_finish_callbacks.insert(0, callback) | ||
|
||
def get_metrics(self) -> _MetricDictType: | ||
def get_metrics(self) -> dict[str, NumericType]: | ||
"""Return all metrics.""" | ||
return self._metrics.copy() | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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... 🤷🏻