use BoundedAttributes for attributes in link, event, resource, spans #1915
Conversation
codeboten
requested review from
aabmass and
ocelotl
and removed request for
a team
| def __init__( | ||
| self, | ||
| maxlen: Optional[int] = _DEFAULT_LIMIT, | ||
| attributes: types.Attributes = None, |
There was a problem hiding this comment.
attributes makes sense in terms of what we are using BoundedDict for, but for the class itself it doesn't make much sense. Should we have a different name instead of BoundedDict?
There was a problem hiding this comment.
It would be good to make a separation from the SDK BoundedDict too.
There was a problem hiding this comment.
Yeah I considered for a minute making Attributes a class to replace BoundedDict, wasn't sure if this would be doable without breaking Attributes' current interface though
There was a problem hiding this comment.
Maybe just BoundedAttributes or something?
| @@ -555,6 +561,16 @@ def __init__( | |||
| OTEL_SPAN_LINK_COUNT_LIMIT, | |||
| _DEFAULT_OTEL_SPAN_LINK_COUNT_LIMIT, | |||
| ) | |||
| self.max_event_attributes = self._from_env_if_absent( | |||
| max_event_attributes, | |||
| OTEL_SPAN_LINK_COUNT_LIMIT, | |||
The functionality seems very specific to |
| 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.
self._dict.popitem(last=False)?
There was a problem hiding this comment.
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.
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.
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.
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
codeboten
changed the title
ocelotl
added
the
Approve Public API check
Description
Continuing towards making attributes consistent across Span, Link, Event and Resource by utilizing BoundedDict everywhere. Added
BoundedDictto the API to make it available forLinkwhich is defined in the API. MarkedBoundedDictin the SDK as deprecated as a result. This is getting us one step closer to supporting dropped attribute counts in the exporters. One question I have for reviewers is whether the BoundedDict should be inopentelemetry.attributesor not, it could live in utils or something like that instead.Fixes #1906
Type of change
Please delete options that are not relevant.
Does This PR Require a Contrib Repo Change?
Checklist: