Add support for `OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT` env var by owais · Pull Request #2056 · open-telemetry/opentelemetry-python

owais · 2021-08-18T22:47:37Z

Description

Fixes #2051

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?

Manually
Added/updated tests

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

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

Fixes open-telemetry#2051

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

Builds on top of #2044. Should be rebased and reviewed after 2044 is merged.

opentelemetry-sdk/tests/trace/test_trace.py

srikanthccv · 2021-08-19T18:51:10Z

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

            OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_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,
+        )


This block of code is confusing to me. I will pull this to local and check.

Wow this whole limits stuff is confusing. Please check the L596-L610 once.

My bad. I think it happened due to a bad rebase on github. Fixed now.

lzchen · 2021-08-23T16:22:13Z

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

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


Shouldn't 543 and 542 be switched?

Model specific if set

Global if set

Model default

Global default

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

lzchen · 2021-08-23T16:26:39Z

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

+"""
+
 OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT = "OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT"
 """


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.

lzchen · 2021-08-23T16:33:02Z

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


    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(


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

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

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

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.

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

Oh, I see it now :)

lzchen

Just a few comments about comments :)

srikanthccv · 2021-08-24T14:19:10Z

opentelemetry-sdk/tests/trace/test_trace.py

        self.assertEqual(2, span.links[0].attributes.dropped)
        self.assertEqual(2, span.resource.attributes.dropped)
+
+    def _test_span_limits(


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

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

owais added 2 commits August 17, 2021 20:20

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

wip

a60f030

owais requested review from a team, NathanielRN and codeboten and removed request for a team August 18, 2021 22:47

Add support for OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT env var

6dd996a

Fixes open-telemetry#2051

owais force-pushed the fix-2051 branch from ab2e367 to 6dd996a Compare August 18, 2021 22:48

owais force-pushed the fix-2051 branch from 4d32d1f to 31d3e0a Compare August 19, 2021 14:30

Merge branch 'main' into fix-2051

4bbfb01

owais force-pushed the fix-2051 branch from 31d3e0a to 4bbfb01 Compare August 19, 2021 14:30

srikanthccv reviewed Aug 19, 2021

View reviewed changes

opentelemetry-sdk/tests/trace/test_trace.py Show resolved Hide resolved

srikanthccv reviewed Aug 19, 2021

View reviewed changes

owais force-pushed the fix-2051 branch from bb5096d to 967a730 Compare August 19, 2021 20:28

Merge branch 'main' into fix-2051

50daa9e

owais force-pushed the fix-2051 branch from 967a730 to 50daa9e Compare August 19, 2021 20:56

owais requested a review from srikanthccv August 19, 2021 20:56

srikanthccv approved these changes Aug 20, 2021

View reviewed changes

Merge branch 'main' into fix-2051

305ce60

lzchen reviewed Aug 23, 2021

View reviewed changes

lzchen approved these changes Aug 23, 2021

View reviewed changes

owais added 2 commits August 24, 2021 18:58

Merge branch 'main' into fix-2051

6c200aa

Merge branch 'main' into fix-2051

545e36c

owais requested review from lzchen and srikanthccv August 24, 2021 13:56

owais added 2 commits August 24, 2021 19:28

Fixed lint issues

0cfd038

Updated doc string

8eb4188

updates spanlimits repr

319fd4b

srikanthccv reviewed Aug 24, 2021

View reviewed changes

fixed bad rebase

10bae11

srikanthccv approved these changes Aug 24, 2021

View reviewed changes

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

owais merged commit 4250078 into open-telemetry:main Aug 24, 2021

owais deleted the fix-2051 branch August 24, 2021 14:54

Conversation

owais commented Aug 18, 2021

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

Uh oh!

owais commented Aug 18, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owais Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

owais Aug 19, 2021 •

edited

Loading

lzchen Aug 23, 2021 •

edited

Loading