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

Allow span limits to be unset #1830

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
- Added example for running Django with auto instrumentation.
([#1803](https://github.com/open-telemetry/opentelemetry-python/pull/1803))
- Allow span limits (OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT, OTEL_SPAN_EVENT_COUNT_LIMIT, OTEL_SPAN_LINK_COUNT_LIMIT)
to be set to "none".
([#1830](https://github.com/open-telemetry/opentelemetry-python/pull/1830))
- Added support for OTEL_SERVICE_NAME.
([#1829](https://github.com/open-telemetry/opentelemetry-python/pull/1829))

Expand Down
19 changes: 15 additions & 4 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,23 @@

logger = logging.getLogger(__name__)

SPAN_ATTRIBUTE_COUNT_LIMIT = int(
environ.get(OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT, 128)

_DEFAULT_SPAN_LIMIT = "128"


def _get_limit_from_env(limit_name) -> Optional[int]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having an "unset" value, should we maybe allow users to specify a limit manually themselves through the span api? This follows the whole hardcoded -> env var -> default value priority architecture that we use in a lot of places for configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be very nice to have. Any configuration that happen automatically on importing modules should be moved to SDK initialization IMO. That said, this PR is merely allowing setting the limits to "none" in addition to integer values.

Created an issue to implement what you suggested: #1838

Will work on it soon but we shouldn't rush it into the release tomorrow.

value = environ.get(limit_name, _DEFAULT_SPAN_LIMIT).strip().lower()
if value == "none":
return None
return int(value)


SPAN_ATTRIBUTE_COUNT_LIMIT = _get_limit_from_env(
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT
)

_SPAN_EVENT_COUNT_LIMIT = int(environ.get(OTEL_SPAN_EVENT_COUNT_LIMIT, 128))
_SPAN_LINK_COUNT_LIMIT = int(environ.get(OTEL_SPAN_LINK_COUNT_LIMIT, 128))
_SPAN_EVENT_COUNT_LIMIT = _get_limit_from_env(OTEL_SPAN_EVENT_COUNT_LIMIT)
_SPAN_LINK_COUNT_LIMIT = _get_limit_from_env(OTEL_SPAN_LINK_COUNT_LIMIT)
_VALID_ATTR_VALUE_TYPES = (bool, str, int, float)
# pylint: disable=protected-access
_TRACE_SAMPLER = sampling._get_from_env_or_default()
Expand Down
15 changes: 8 additions & 7 deletions opentelemetry-sdk/src/opentelemetry/sdk/util/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def extend(self, seq):
@classmethod
def from_seq(cls, maxlen, seq):
seq = tuple(seq)
if len(seq) > maxlen:
if maxlen is not None and len(seq) > maxlen:
raise ValueError
bounded_list = cls(maxlen)
# pylint: disable=protected-access
Expand All @@ -97,10 +97,11 @@ class BoundedDict(MutableMapping):
"""

def __init__(self, maxlen):
if not isinstance(maxlen, int):
raise ValueError
if maxlen < 0:
raise ValueError
if maxlen is not None:
if not isinstance(maxlen, int):
raise ValueError
if maxlen < 0:
raise ValueError
self.maxlen = maxlen
self.dropped = 0
self._dict = OrderedDict() # type: OrderedDict
Expand All @@ -122,7 +123,7 @@ def __setitem__(self, key, value):

if key in self._dict:
del self._dict[key]
elif len(self._dict) == self.maxlen:
elif self.maxlen is not None and len(self._dict) == self.maxlen:
del self._dict[next(iter(self._dict.keys()))]
self.dropped += 1
self._dict[key] = value
Expand All @@ -140,7 +141,7 @@ def __len__(self):
@classmethod
def from_map(cls, maxlen, mapping):
mapping = OrderedDict(mapping)
if len(mapping) > maxlen:
if maxlen is not None and len(mapping) > maxlen:
raise ValueError
bounded_dict = cls(maxlen)
# pylint: disable=protected-access
Expand Down
16 changes: 16 additions & 0 deletions opentelemetry-sdk/tests/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,14 @@ def test_extend_drop(self):
self.assertEqual(len(blist), list_len)
self.assertEqual(blist.dropped, len(other_list))

def test_no_limit(self):
blist = BoundedList(maxlen=None)
for num in range(100):
blist.append(num)

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


class TestBoundedDict(unittest.TestCase):
base = collections.OrderedDict(
Expand Down Expand Up @@ -211,3 +219,11 @@ def test_bounded_dict(self):

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

def test_no_limit_code(self):
bdict = BoundedDict(maxlen=None)
for num in range(100):
bdict[num] = num

for num in range(100):
self.assertEqual(bdict[num], num)
38 changes: 38 additions & 0 deletions opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import unittest
from importlib import reload
from logging import ERROR, WARNING
from random import randint
from typing import Optional
from unittest import mock

Expand Down Expand Up @@ -1306,3 +1307,40 @@ def test_span_environment_limits(self):

self.assertEqual(len(root.attributes), 10)
self.assertEqual(len(root.events), 20)

@mock.patch.dict(
"os.environ",
{
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: "none",
OTEL_SPAN_EVENT_COUNT_LIMIT: "none",
OTEL_SPAN_LINK_COUNT_LIMIT: "none",
},
)
def test_span_environment_limits_set_to_none(self):
# pylint: disable=protected-access
num_links = num_attrs_events = int(
trace._DEFAULT_SPAN_LIMIT
) + randint(1, 100)
reload(trace)
tracer = new_tracer()
id_generator = RandomIdGenerator()
some_links = [
trace_api.Link(
trace_api.SpanContext(
trace_id=id_generator.generate_trace_id(),
span_id=id_generator.generate_span_id(),
is_remote=False,
)
)
for _ in range(num_links)
]
with tracer.start_as_current_span("root", links=some_links) as root:
self.assertEqual(len(root.links), num_links)

with tracer.start_as_current_span("root") as root:
for idx in range(num_attrs_events):
root.set_attribute("my_attribute_{}".format(idx), 0)
root.add_event("my_event_{}".format(idx))

self.assertEqual(len(root.attributes), num_attrs_events)
self.assertEqual(len(root.events), num_attrs_events)