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.

Sign up for GitHub

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

Jump to bottom

Add support for OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT env var #2056

Merged
merged 12 commits into from
Aug 24, 2021
Merged

Add support for OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT env var #2056

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
061e72c
Added support for `OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT`
owais Aug 17, 2021
a60f030
wip
owais Aug 18, 2021
6dd996a
Add support for `OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT` env var
owais Aug 18, 2021
4bbfb01
Merge branch 'main' into fix-2051
owais Aug 19, 2021
50daa9e
Merge branch 'main' into fix-2051
owais Aug 19, 2021
305ce60
Merge branch 'main' into fix-2051
owais Aug 20, 2021
6c200aa
Merge branch 'main' into fix-2051
owais Aug 24, 2021
545e36c
Merge branch 'main' into fix-2051
owais Aug 24, 2021
0cfd038
Fixed lint issues
owais Aug 24, 2021
8eb4188
Updated doc string
owais Aug 24, 2021
319fd4b
updates spanlimits repr
owais Aug 24, 2021
10bae11
fixed bad rebase
owais Aug 24, 2021
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#2044](https://github.com/open-telemetry/opentelemetry-python/pull/2044))
- `opentelemetry-sdk` Fixed bugs (#2041, #2042 & #2045) in Span Limits
([#2044](https://github.com/open-telemetry/opentelemetry-python/pull/2044))
- `opentelemetry-sdk` Add support for `OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT` env var
([#2056](https://github.com/open-telemetry/opentelemetry-python/pull/2056))
- `opentelemetry-api` Attribute keys must be non-empty strings.
([#2057](https://github.com/open-telemetry/opentelemetry-python/pull/2057))

Expand Down
7 changes: 7 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/environment_variables/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@
Default: 512
"""

OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT = "OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT"
"""
.. envvar:: OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT

The :envvar:`OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT` represents the maximum allowed attribute length.
"""

OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT = "OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT"
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you modify the docstring for OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT to say that it is specific for attributes on span? AS well, it takes precedence over OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT specifically for span attributes.

.. envvar:: OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT
Expand Down
23 changes: 21 additions & 2 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
from opentelemetry.attributes import BoundedAttributes
from opentelemetry.sdk import util
from opentelemetry.sdk.environment_variables import (
OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT,
OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT,
OTEL_LINK_ATTRIBUTE_COUNT_LIMIT,
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT,
Expand Down Expand Up @@ -535,6 +536,12 @@ class SpanLimits:
environment variable.
- If the environment variable is not set, the default value for the limit is used.

Limit precedence:

- If a model specific limit is set, it will be used.
- Else if the model specific limit has a default value, the default value will be used.
- Else if model specific limit has a corresponding global limit, the global limit will be used.
Copy link
Contributor

@lzchen lzchen Aug 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't 543 and 542 be switched?

  1. Model specific if set
  2. Global if set
  3. Model default
  4. Global default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They way I understood the spec, I think model default takes precedence over global user provided value and it sure is counter-intuitive. Spec says the following:

An SDK MAY implement model-specific limits, for example SpanAttributeCountLimit. If both a general and a model-specific limit are implemented, then the SDK MUST first attempt to use the model-specific limit, if it isn't set and doesn't have a default, then the SDK MUST attempt to use the general limit.

Am I reading it wrong?

Also created a spec issue here to get clarification: open-telemetry/opentelemetry-specification#1878


Args:
max_attributes: Maximum number of attributes that can be added to a Span.
Environment variable: OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT
Expand All @@ -551,6 +558,8 @@ class SpanLimits:
Default: {_DEFAULT_OTEL_LINK_ATTRIBUTE_COUNT_LIMIT}
max_attribute_length: Maximum length an attribute value can have. Values longer than
the specified length will be truncated.
max_span_attribute_length: Maximum length a span attribute value can have. Values longer than
the specified length will be truncated.
"""

UNSET = -1
Expand All @@ -563,6 +572,7 @@ def __init__(
max_event_attributes: Optional[int] = None,
max_link_attributes: Optional[int] = None,
max_attribute_length: Optional[int] = None,
max_span_attribute_length: Optional[int] = None,
):
self.max_attributes = self._from_env_if_absent(
max_attributes,
Expand All @@ -589,20 +599,28 @@ def __init__(
OTEL_LINK_ATTRIBUTE_COUNT_LIMIT,
_DEFAULT_OTEL_LINK_ATTRIBUTE_COUNT_LIMIT,
)

self.max_attribute_length = self._from_env_if_absent(
max_attribute_length,
OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT,
)
self.max_span_attribute_length = self._from_env_if_absent(
max_span_attribute_length,
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT,
# use global attribute length limit as default
self.max_attribute_length,
)

def __repr__(self):
return "{}(max_attributes={}, max_events={}, max_links={}, max_event_attributes={}, max_link_attributes={}, max_attribute_length={})".format(
return "{}(max_attributes={}, max_events={}, max_links={}, max_event_attributes={}, max_link_attributes={}, max_attribute_length={}, max_span_attribute_length={})".format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we switch the ordering of span_attribute and attribute? I'd like to see the model specific be defined first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't that break the API (change behavior) if all arguments are passed as positional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh do you mean if someone were to call __repr__ directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I mean if we swap max_span_attributes with max_attributes the signature will look like:

SpanLimits(max_span_attributes, max_events, max_links, max_event_attributes, max_link_attributes, max_attributes)

which would be model, global, global, model, model, global. That doesn't look very nice. At least all max_*_attributes should be together. If we change the signature to

SpanLimits(max_span_attributes, max_event_attributes, max_link_attributes, max_attributes, max_links, max_events)

Then any users calling the function today as SpanLimits(1, 2, 3, 4, 5) will get unexpected behavior that'll be hard to detect as we changed the position of arguments.

Copy link
Member

Choose a reason for hiding this comment

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

I think @lzchen didn't mean to change the order in __init__ but just in the __repr__ string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see it now :)

type(self).__name__,
self.max_attributes,
self.max_events,
self.max_links,
self.max_event_attributes,
self.max_link_attributes,
self.max_attribute_length,
self.max_span_attribute_length,
)

@classmethod
Expand Down Expand Up @@ -638,6 +656,7 @@ def _from_env_if_absent(
max_event_attributes=SpanLimits.UNSET,
max_link_attributes=SpanLimits.UNSET,
max_attribute_length=SpanLimits.UNSET,
max_span_attribute_length=SpanLimits.UNSET,
)

# not remove for backward compat. please use SpanLimits instead.
Expand Down Expand Up @@ -713,7 +732,7 @@ def __init__(
self._limits.max_attributes,
attributes,
immutable=False,
max_value_len=self._limits.max_attribute_length,
max_value_len=self._limits.max_span_attribute_length,
)
self._events = self._new_events()
if events:
Expand Down
133 changes: 131 additions & 2 deletions opentelemetry-sdk/tests/trace/test_trace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from opentelemetry.context import Context
from opentelemetry.sdk import resources, trace
from opentelemetry.sdk.environment_variables import (
OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT,
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT,
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT,
OTEL_SPAN_EVENT_COUNT_LIMIT,
Expand Down Expand Up @@ -1335,6 +1336,25 @@ def test_limits_defaults(self):
limits.max_links, trace._DEFAULT_OTEL_SPAN_LINK_COUNT_LIMIT
)
self.assertIsNone(limits.max_attribute_length)
self.assertIsNone(limits.max_span_attribute_length)

def test_limits_attribute_length_limits_code(self):
# global limit unset while span limit is set
limits = trace.SpanLimits(max_span_attribute_length=22)
self.assertIsNone(limits.max_attribute_length)
self.assertEqual(limits.max_span_attribute_length, 22)

# span limit falls back to global limit when no value is provided
limits = trace.SpanLimits(max_attribute_length=22)
self.assertEqual(limits.max_attribute_length, 22)
self.assertEqual(limits.max_span_attribute_length, 22)

# global and span limits set to different values
limits = trace.SpanLimits(
max_attribute_length=22, max_span_attribute_length=33
)
self.assertEqual(limits.max_attribute_length, 22)
self.assertEqual(limits.max_span_attribute_length, 33)

def test_limits_values_code(self):
max_attributes, max_events, max_links, max_attr_length = (
Expand Down Expand Up @@ -1477,7 +1497,8 @@ def _test_span_no_limits(self, tracer):
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: "13",
OTEL_SPAN_EVENT_COUNT_LIMIT: "7",
OTEL_SPAN_LINK_COUNT_LIMIT: "4",
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT: "11",
OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT: "11",
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT: "15",
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
},
)
def test_span_limits_env(self):
Expand All @@ -1487,6 +1508,7 @@ def test_span_limits_env(self):
max_events=7,
max_links=4,
max_attr_len=11,
max_span_attr_len=15,
)

@mock.patch.dict(
Expand All @@ -1495,7 +1517,8 @@ def test_span_limits_env(self):
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: "10",
OTEL_SPAN_EVENT_COUNT_LIMIT: "20",
OTEL_SPAN_LINK_COUNT_LIMIT: "30",
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT: "40",
OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT: "40",
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT: "50",
},
)
def test_span_limits_default_to_env(self):
Expand All @@ -1506,12 +1529,14 @@ def test_span_limits_default_to_env(self):
max_events=None,
max_links=None,
max_attribute_length=None,
max_span_attribute_length=None,
)
),
max_attrs=10,
max_events=20,
max_links=30,
max_attr_len=40,
max_span_attr_len=50,
)

def test_span_limits_code(self):
Expand All @@ -1522,12 +1547,14 @@ def test_span_limits_code(self):
max_events=15,
max_links=13,
max_attribute_length=9,
max_span_attribute_length=25,
)
),
max_attrs=11,
max_events=15,
max_links=13,
max_attr_len=9,
max_span_attr_len=25,
)

@mock.patch.dict(
Expand Down Expand Up @@ -1562,3 +1589,105 @@ def test_dropped_attributes(self):
self.assertEqual(2, span.events[0].attributes.dropped)
self.assertEqual(2, span.links[0].attributes.dropped)
self.assertEqual(2, span.resource.attributes.dropped)

def _test_span_limits(
Copy link
Member

Choose a reason for hiding this comment

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

Is this because of bad rebase? Why is it showing as new addition if not used anywhere in this diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I had moved them to the bottom but looks like rebase added them back to the original place? Fixed now.

self,
tracer,
max_attrs,
max_events,
max_links,
max_attr_len,
max_span_attr_len,
):
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,
),
attributes={"k": self.long_val},
)
for _ in range(100)
]

some_attrs = {
"init_attribute_{}".format(idx): self.long_val
for idx in range(100)
}
with tracer.start_as_current_span(
"root", links=some_links, attributes=some_attrs
) as root:
self.assertEqual(len(root.links), max_links)
self.assertEqual(len(root.attributes), max_attrs)
for idx in range(100):
root.set_attribute(
"my_str_attribute_{}".format(idx), self.long_val
)
root.set_attribute(
"my_byte_attribute_{}".format(idx), self.long_val.encode()
)
root.set_attribute(
"my_int_attribute_{}".format(idx), self.long_val.encode()
)
root.add_event(
"my_event_{}".format(idx), attributes={"k": self.long_val}
)

self.assertEqual(len(root.attributes), max_attrs)
self.assertEqual(len(root.events), max_events)

for link in root.links:
for attr_val in link.attributes.values():
self._assert_attr_length(attr_val, max_attr_len)

for event in root.events:
for attr_val in event.attributes.values():
self._assert_attr_length(attr_val, max_attr_len)

for attr_val in root.attributes.values():
self._assert_attr_length(attr_val, max_span_attr_len)

def _test_span_no_limits(self, tracer):
num_links = int(trace._DEFAULT_OTEL_SPAN_LINK_COUNT_LIMIT) + randint(
1, 100
)

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)

num_events = int(trace._DEFAULT_OTEL_SPAN_EVENT_COUNT_LIMIT) + randint(
1, 100
)
with tracer.start_as_current_span("root") as root:
for idx in range(num_events):
root.add_event(
"my_event_{}".format(idx), attributes={"k": self.long_val}
)

self.assertEqual(len(root.events), num_events)

num_attributes = int(
trace._DEFAULT_OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT
) + randint(1, 100)
with tracer.start_as_current_span("root") as root:
for idx in range(num_attributes):
root.set_attribute(
"my_attribute_{}".format(idx), self.long_val
)

self.assertEqual(len(root.attributes), num_attributes)
for attr_val in root.attributes.values():
self.assertEqual(attr_val, self.long_val)