Add support for additional Span Limits #2044

owais · 2021-08-17T09:20:14Z

Description

Added support for OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT, OTEL_LINK_ATTRIBUTE_COUNT_LIMIT & OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT, and fixes a number of bugs.

Fixes #2045
Fixes #2043
Fixes #2042
Fixes #2041

Otel Specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#span-limits-

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added/updated tests
Manual testing

Does This PR Require a Contrib Repo Change?

Yes. - Link to PR:
No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

opentelemetry-api/src/opentelemetry/attributes/__init__.py

owais · 2021-08-17T09:51:09Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

-                            "Byte attribute could not be decoded for key `%s`.",
-                            key,
-                        )
-                        return


these checks were redundant as they're run by BoundedAttributes internally as well.

Fixes open-telemetry#2045 Fixes open-telemetry#2043 Fixes open-telemetry#2042 Fixes open-telemetry#2041

owais · 2021-08-17T15:04:49Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+        self._resource._attributes = BoundedAttributes(
+            self._span_limits.max_attributes,
+            self._resource._attributes,
+            max_value_len=self._span_limits.max_attribute_length,


This follows the existing pattern used with span events but technically mutates the resource as it'll end up truncating or dropping pre-existing attributes on a resource. I think in very strict terms it does mutate the resource but in practice this is what users would expect IMO. Curious to hear what other think about this.

I kind of confused why span limits are used with resource. We don't want to mutate resource since it is prohibited by the spec and I interpreted Resource as its own entity different from span/metric/log. Did the spec introduce limits for Resources?

Spec doesn't say anything about it AFAIK and I agree we can probably exclude resources completely. In practice the issues that limits solve don't really plague resources anyway. However, we were applying these limits (the default one at least) so far so this will be a behavioral change (128 max resource attributes to unlimited) although extremely unlikely to impact anyone. @lonewolf3739

I personally don't find using span limits for resources to be good. Can we take this up in separate issue?

I agree with you. Let's discuss in the SIG meeting.

lzchen · 2021-08-18T17:34:08Z

opentelemetry-api/src/opentelemetry/attributes/__init__.py


-
-_DEFAULT_LIMIT = 128
+    if limit is not None and isinstance(value, str):


Could the attribute length truncation technically apply to other value types as well?

Spec limits it to only strings and arrays of strings where the limit applies to each element. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/common.md#attribute-limits

if it is a string, if it exceeds that limit (counting any character in it as 1), SDKs MUST truncate that value, so that its length is at most equal to the limit,
if it is an array of strings, then apply the above rule to each of the values separately,
otherwise a value MUST NOT be truncated;

lzchen · 2021-08-18T17:37:11Z

opentelemetry-api/src/opentelemetry/attributes/__init__.py

@@ -129,10 +148,10 @@ def __init__(
                )
        self.maxlen = maxlen
        self.dropped = 0
+        self.max_value_len = max_value_len


Might need some validation for: Empty value is treated as infinity. Non-integer and negative values are invalid.

I know this is a small ask but I ended up fixing too many unrelated issues in this PR. Created an issue for this and will take care of it in a separate one tomorrow. #2052

lzchen · 2021-08-18T17:45:58Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -630,8 +637,10 @@ def _from_env_if_absent(
    max_links=SpanLimits.UNSET,
    max_event_attributes=SpanLimits.UNSET,
    max_link_attributes=SpanLimits.UNSET,
+    max_attribute_length=SpanLimits.UNSET,


For some reason this limit is the only one in the specs to have notes. Negative values are invalid apparently? What do we do in those cases.

I'm sure that note applies to all limits and it was just a mistake by the author. I'll take care of negative/unset values in a new PR (created issue: #2052)

owais · 2021-08-18T22:49:51Z

@lzchen @lonewolf3739 please review public API check and add the label if everything looks good.

owais · 2021-08-19T13:56:20Z

Public API check was approved on another PR with these changes so I'm self-approving the check here.

owais requested a review from a team as a code owner August 17, 2021 09:20

owais requested review from ocelotl and NathanielRN and removed request for a team August 17, 2021 09:20

owais force-pushed the span-attr-length-limit branch 7 times, most recently from b8c6ce2 to 6f25751 Compare August 17, 2021 09:49

owais commented Aug 17, 2021

View reviewed changes

owais mentioned this pull request Aug 17, 2021

Support OTEL_SPAN_ATTRIBUTE_SIZE_LIMIT #1847

Closed

11 tasks

owais force-pushed the span-attr-length-limit branch 2 times, most recently from 5c8ef2e to 14d3e46 Compare August 17, 2021 10:18

Added support for OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT

061e72c

Fixes open-telemetry#2045 Fixes open-telemetry#2043 Fixes open-telemetry#2042 Fixes open-telemetry#2041

owais force-pushed the span-attr-length-limit branch from 14d3e46 to 061e72c Compare August 17, 2021 14:50

owais changed the title ~~Add support for OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT~~ Add support for OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT, OTEL_LINK_ATTRIBUTE_COUNT_LIMIT & OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT Aug 17, 2021

owais changed the title ~~Add support for OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT, OTEL_LINK_ATTRIBUTE_COUNT_LIMIT & OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT~~ Add support for additional Span Limits Aug 17, 2021

owais commented Aug 17, 2021

View reviewed changes

owais mentioned this pull request Aug 18, 2021

Add support for missing environment variables to SDK #2035

Open

27 tasks

lzchen reviewed Aug 18, 2021

View reviewed changes

lzchen approved these changes Aug 18, 2021

View reviewed changes

srikanthccv approved these changes Aug 18, 2021

View reviewed changes

This was referenced Aug 18, 2021

Fix limit env vars unset/unlimited values #2054

Merged

Add support for OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT env var #2056

Merged

owais added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Aug 19, 2021

owais merged commit 2a726e4 into open-telemetry:main Aug 19, 2021

owais deleted the span-attr-length-limit branch August 19, 2021 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for additional Span Limits #2044

Add support for additional Span Limits #2044

owais commented Aug 17, 2021 •

edited

owais Aug 17, 2021

owais Aug 17, 2021

srikanthccv Aug 18, 2021 •

edited

owais Aug 18, 2021 •

edited

srikanthccv Aug 18, 2021

owais Aug 18, 2021 •

edited

lzchen Aug 18, 2021

owais Aug 18, 2021

lzchen Aug 18, 2021

owais Aug 18, 2021

lzchen Aug 18, 2021

owais Aug 18, 2021

owais commented Aug 18, 2021

owais commented Aug 19, 2021



		_DEFAULT_LIMIT = 128
		if limit is not None and isinstance(value, str):

Add support for additional Span Limits #2044

Add support for additional Span Limits #2044

Conversation

owais commented Aug 17, 2021 • edited

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikanthccv Aug 18, 2021 • edited

Choose a reason for hiding this comment

owais Aug 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais Aug 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais commented Aug 18, 2021

owais commented Aug 19, 2021

owais commented Aug 17, 2021 •

edited

srikanthccv Aug 18, 2021 •

edited

owais Aug 18, 2021 •

edited

owais Aug 18, 2021 •

edited