Skip to content
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

Merged
merged 14 commits into from
Jun 25, 2021
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Attributes for `Link` and `Resource` are immutable as they are for `Event`, which means
any attempt to modify attributes directly will result in a `TypeError` exception.
([#1909](https://github.com/open-telemetry/opentelemetry-python/pull/1909))
- Added `BoundedAttributes` to the API to make it available for `Link` which is defined in the
API. Marked `BoundedDict` in the SDK as deprecated as a result.
([#1915](https://github.com/open-telemetry/opentelemetry-python/pull/1915))

## [1.3.0-0.22b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.3.0-0.22b0) - 2021-06-01

Expand Down
78 changes: 73 additions & 5 deletions opentelemetry-api/src/opentelemetry/attributes/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 BoundedAttributes(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,
Copy link
Contributor

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 using BoundedDict for, but for the class itself it doesn't make much sense. Should we have a different name instead of BoundedDict?

Copy link
Contributor

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.

Copy link
Contributor Author

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 replace BoundedDict, wasn't sure if this would be doable without breaking Attributes' current interface though

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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()))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._dict.popitem(last=False)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
8 changes: 3 additions & 5 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,7 @@
from typing import Iterator, Optional, Sequence, cast

from opentelemetry import context as context_api
from opentelemetry.attributes import ( # type: ignore
_create_immutable_attributes,
)
from opentelemetry.attributes import BoundedAttributes # type: ignore
from opentelemetry.context.context import Context
from opentelemetry.environment_variables import OTEL_PYTHON_TRACER_PROVIDER
from opentelemetry.trace.propagation import (
Expand Down Expand Up @@ -142,8 +140,8 @@ def __init__(
attributes: types.Attributes = None,
) -> None:
super().__init__(context)
self._attributes = _create_immutable_attributes(
attributes
self._attributes = BoundedAttributes(
attributes=attributes
) # type: types.Attributes

@property
Expand Down
99 changes: 93 additions & 6 deletions opentelemetry-api/tests/attributes/test_attributes.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@

# type: ignore

import collections
import unittest

from opentelemetry.attributes import (
_create_immutable_attributes,
BoundedAttributes,
_filter_attributes,
_is_valid_attribute_value,
)
Expand Down Expand Up @@ -79,9 +80,95 @@ def test_filter_attributes(self):
},
)

def test_create_immutable_attributes(self):
attrs = {"key": "value", "pi": 3.14}
immutable = _create_immutable_attributes(attrs)
# TypeError: 'mappingproxy' object does not support item assignment

class TestBoundedAttributes(unittest.TestCase):
base = collections.OrderedDict(
[
("name", "Firulais"),
("age", 7),
("weight", 13),
("vaccinated", True),
]
)

def test_negative_maxlen(self):
with self.assertRaises(ValueError):
BoundedAttributes(-1)

def test_from_map(self):
dic_len = len(self.base)
base_copy = collections.OrderedDict(self.base)
bdict = BoundedAttributes(dic_len, base_copy)

self.assertEqual(len(bdict), dic_len)

# modify base_copy and test that bdict is not changed
base_copy["name"] = "Bruno"
base_copy["age"] = 3

for key in self.base:
self.assertEqual(bdict[key], self.base[key])

# test that iter yields the correct number of elements
self.assertEqual(len(tuple(bdict)), dic_len)

# map too big
half_len = dic_len // 2
bdict = BoundedAttributes(half_len, self.base)
self.assertEqual(len(tuple(bdict)), half_len)
self.assertEqual(bdict.dropped, dic_len - half_len)

def test_bounded_dict(self):
# create empty dict
dic_len = len(self.base)
bdict = BoundedAttributes(dic_len, immutable=False)
self.assertEqual(len(bdict), 0)

# fill dict
for key in self.base:
bdict[key] = self.base[key]

self.assertEqual(len(bdict), dic_len)
self.assertEqual(bdict.dropped, 0)

for key in self.base:
self.assertEqual(bdict[key], self.base[key])

# test __iter__ in BoundedAttributes
for key in bdict:
self.assertEqual(bdict[key], self.base[key])

# updating an existing element should not drop
bdict["name"] = "Bruno"
self.assertEqual(bdict.dropped, 0)

# try to append more elements
for key in self.base:
bdict["new-" + key] = self.base[key]

self.assertEqual(len(bdict), dic_len)
self.assertEqual(bdict.dropped, dic_len)

# test that elements in the dict are the new ones
for key in self.base:
self.assertEqual(bdict["new-" + key], self.base[key])

# delete an element
del bdict["new-name"]
self.assertEqual(len(bdict), dic_len - 1)

with self.assertRaises(KeyError):
_ = bdict["new-name"]

def test_no_limit_code(self):
bdict = BoundedAttributes(maxlen=None, immutable=False)
for num in range(100):
bdict[num] = num

for num in range(100):
self.assertEqual(bdict[num], num)

def test_immutable(self):
bdict = BoundedAttributes()
with self.assertRaises(TypeError):
immutable["pi"] = 1.34
bdict["should-not-work"] = "dict immutable"
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,7 @@

import pkg_resources

from opentelemetry.attributes import (
_create_immutable_attributes,
_filter_attributes,
)
from opentelemetry.attributes import BoundedAttributes
from opentelemetry.sdk.environment_variables import (
OTEL_RESOURCE_ATTRIBUTES,
OTEL_SERVICE_NAME,
Expand Down Expand Up @@ -147,8 +144,7 @@ class Resource:
def __init__(
self, attributes: Attributes, schema_url: typing.Optional[str] = None
):
_filter_attributes(attributes)
self._attributes = _create_immutable_attributes(attributes)
self._attributes = BoundedAttributes(attributes=attributes)
if schema_url is None:
schema_url = ""
self._schema_url = schema_url
Expand Down
Loading