-
Notifications
You must be signed in to change notification settings - Fork 657
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
use BoundedAttributes for attributes in link, event, resource, spans #1915
Changes from 12 commits
e7550e7
f237bae
a8e923a
c00d04a
09ca0a3
9bd9596
736500d
c9662ed
943b078
f05094d
f29c775
48a378d
8a84a0a
1b98aa9
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 |
---|---|---|
|
@@ -14,8 +14,11 @@ | |
# type: ignore | ||
|
||
import logging | ||
import threading | ||
from collections import OrderedDict | ||
from collections.abc import MutableMapping | ||
from types import MappingProxyType | ||
from typing import MutableSequence, Sequence | ||
from typing import MutableSequence, Optional, Sequence | ||
|
||
from opentelemetry.util import types | ||
|
||
|
@@ -104,7 +107,72 @@ def _filter_attributes(attributes: types.Attributes) -> None: | |
attributes.pop(attr_key) | ||
|
||
|
||
def _create_immutable_attributes( | ||
attributes: types.Attributes, | ||
) -> types.Attributes: | ||
return MappingProxyType(attributes.copy() if attributes else {}) | ||
_DEFAULT_LIMIT = 128 | ||
|
||
|
||
class BoundedDict(MutableMapping): | ||
"""An ordered dict with a fixed max capacity. | ||
|
||
Oldest elements are dropped when the dict is full and a new element is | ||
added. | ||
""" | ||
|
||
def __init__( | ||
self, | ||
maxlen: Optional[int] = _DEFAULT_LIMIT, | ||
attributes: types.Attributes = None, | ||
immutable: bool = True, | ||
): | ||
if maxlen is not None: | ||
if not isinstance(maxlen, int) or maxlen < 0: | ||
raise ValueError( | ||
"maxlen must be valid int greater or equal to 0" | ||
) | ||
self.maxlen = maxlen | ||
self.dropped = 0 | ||
self._dict = OrderedDict() # type: OrderedDict | ||
self._lock = threading.Lock() # type: threading.Lock | ||
if attributes: | ||
_filter_attributes(attributes) | ||
for key, value in attributes.items(): | ||
self[key] = value | ||
self._immutable = immutable | ||
|
||
def __repr__(self): | ||
return "{}({}, maxlen={})".format( | ||
type(self).__name__, dict(self._dict), self.maxlen | ||
) | ||
|
||
def __getitem__(self, key): | ||
return self._dict[key] | ||
|
||
def __setitem__(self, key, value): | ||
if getattr(self, "_immutable", False): | ||
raise TypeError | ||
with self._lock: | ||
if self.maxlen is not None and self.maxlen == 0: | ||
self.dropped += 1 | ||
return | ||
|
||
if key in self._dict: | ||
del self._dict[key] | ||
elif self.maxlen is not None and len(self._dict) == self.maxlen: | ||
del self._dict[next(iter(self._dict.keys()))] | ||
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 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'm curious what people's thoughts on this are. It's confusing to me that if the attributes dict is full, we delete an existing item from the dictionary in favour of the new item. Any thoughts @lzchen? 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. Yeah my first impression when reading the specs is that the LATEST item that is attempting to be added would be dropped instead of popping the first item that was added. If we decide on changing the functionality, you could leave that for a separate PR if you wish. 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. What do other SIGs do? And also spec says There SHOULD be a log emitted to indicate to the user that an attribute, event, or link was discarded due to such a limit. To prevent excessive logging, the log should not be emitted once per span, or per discarded attribute, event, or links. https://github.com/open-telemetry/opentelemetry-specification/blob/b46bcab5fb709381f1fd52096a19541370c7d1b3/specification/trace/sdk.md#span-limits 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. Looks like go does the same thing as we do currently. https://github.com/open-telemetry/opentelemetry-go/blob/c1f460e097d395fa7cd02acc4a6096d6c6f14b08/sdk/trace/attributesmap.go#L45-L62 I agree that for now it probably doesn't make sense to change the behaviour |
||
self.dropped += 1 | ||
self._dict[key] = value | ||
|
||
def __delitem__(self, key): | ||
if getattr(self, "_immutable", False): | ||
raise TypeError | ||
with self._lock: | ||
del self._dict[key] | ||
|
||
def __iter__(self): | ||
with self._lock: | ||
return iter(self._dict.copy()) | ||
|
||
def __len__(self): | ||
return len(self._dict) | ||
|
||
def copy(self): | ||
return self._dict.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.
attributes
makes sense in terms of what we are usingBoundedDict
for, but for the class itself it doesn't make much sense. Should we have a different name instead ofBoundedDict
?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.
It would be good to make a separation from the SDK
BoundedDict
too.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.
Yeah I considered for a minute making
Attributes
a class to replaceBoundedDict
, wasn't sure if this would be doable without breaking Attributes' current interface thoughThere 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.
Maybe just
BoundedAttributes
or something?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.
Done.