Fix limit env vars unset/unlimited values by owais · Pull Request #2054 · open-telemetry/opentelemetry-python

owais · 2021-08-18T21:36:15Z

Description

Limit env vars set to an empty value will be treated as unset/unlimited.

Fixes #2052

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 tests
Updated exsiting 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#2052

owais · 2021-08-18T21:38:32Z

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

ocelotl · 2021-08-19T09:24:41Z

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

+        - Its length is greater than the maximum allowed length.
+        - It needs to be encoded/decoded e.g, bytes to strings.


Nit: Looks like no cleaning is needed if value is not string or bytes, if this is checked first then all the remaining execution in this function can be saved.

We also need to clean the string/byte value in sequences (and validate them) so can't bail out

ocelotl · 2021-08-19T09:27:07Z

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

+    if key is None or key == "":
+        _logger.warning("invalid key `%s` (empty or null)", key)
+        return None
+


Suggested change

if isinstance(value, (int, float, bool)):

return True

Fixed in #2057

ocelotl · 2021-08-19T09:46:58Z

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

+    if isinstance(value, bytes):
+        try:
+            value = value.decode()
+        except ValueError:


Suggested change

except ValueError:

except UnicodeDecodeError:

srikanthccv · 2021-08-19T15:02:17Z

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

 _DEFAULT_OTEL_LINK_ATTRIBUTE_COUNT_LIMIT = 128


-_ENV_VALUE_UNSET = "unset"


didn't we use unset so users should define it explicitly and can avoid the sudden surprise when not setting env either by no knowledge/ignorance considered as unlimited?

We did but spec recommends using empty value as unlimited.

We can try to convince others and update spec to accept "unset" as the value but for now I think we should update our SDK to make sure it is spec compatible.

CHANGELOG.md

lzchen · 2021-08-19T22:32:19Z

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

        cls, value: Optional[int], env_var: str, default: Optional[int] = None
    ) -> Optional[int]:
-        if value is cls.UNSET:
+        if value == cls.UNSET:


Maybe I'm not fully understanding this scenario. When will value ever be "unset"? Isn't value supposed to be an int?

The API usage looks like the following in order to not set a limit:

SpanLimits(max_attributes=SpanLimits.UNSET)

SpanLimits.UNSET is a symbol that users can pass as a limit to set the limit to infinity. We can freely change its internal value to anything without breaking any contracts.

The reason why I used "unset" was that I thought -1 might be confusing as the spec disallows it for env vars. Internally we can still use -1 as the symbol value.

Changed it back to -1 as this is probably causing more confusion and does not respect the function contract. I wonder why mypy didn't catch it.

Co-authored-by: Leighton Chen <lechen@microsoft.com>

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 requested review from a team, ocelotl and srikanthccv and removed request for a team August 18, 2021 21:36

owais force-pushed the fix-2052 branch from 891233f to 32390f4 Compare August 18, 2021 21:37

SpanLimit: Treat empty value env vars as unset

18574c4

Fixes open-telemetry#2052

owais force-pushed the fix-2052 branch from 32390f4 to 18574c4 Compare August 18, 2021 21:37

owais changed the title ~~Fix 2052~~ Fix limit env vars unset/unlimited values Aug 18, 2021

ocelotl approved these changes Aug 19, 2021

View reviewed changes

ocelotl 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

Merge branch 'main' into fix-2052

90b361a

srikanthccv reviewed Aug 19, 2021

View reviewed changes

srikanthccv approved these changes Aug 19, 2021

View reviewed changes

Merge branch 'main' into fix-2052

697846f

lzchen reviewed Aug 19, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lzchen reviewed Aug 19, 2021

View reviewed changes

Update CHANGELOG.md

fdd4610

Co-authored-by: Leighton Chen <lechen@microsoft.com>

owais force-pushed the fix-2052 branch from 822c74f to fdd4610 Compare August 20, 2021 00:15

Merge branch 'main' into fix-2052

1ca4c52

lzchen approved these changes Aug 20, 2021

View reviewed changes

Merge branch 'main' into fix-2052

303ed43

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

owais deleted the fix-2052 branch August 24, 2021 13:48

		- Its length is greater than the maximum allowed length.
		- It needs to be encoded/decoded e.g, bytes to strings.

		_DEFAULT_OTEL_LINK_ATTRIBUTE_COUNT_LIMIT = 128


		_ENV_VALUE_UNSET = "unset"

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants