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

Fix limit env vars unset/unlimited values #2054

Merged
merged 7 commits into from
Aug 24, 2021
Merged

Fix limit env vars unset/unlimited values #2054

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
061e72c
Added support for `OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT`
owais Aug 17, 2021
18574c4
SpanLimit: Treat empty value env vars as unset
owais Aug 18, 2021
90b361a
Merge branch 'main' into fix-2052
owais Aug 19, 2021
697846f
Merge branch 'main' into fix-2052
owais Aug 19, 2021
fdd4610
Update CHANGELOG.md
owais Aug 19, 2021
1ca4c52
Merge branch 'main' into fix-2052
lzchen Aug 20, 2021
303ed43
Merge branch 'main' into fix-2052
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` Treat limit even vars set to empty values as unset/unlimited.
([#2054](https://github.com/open-telemetry/opentelemetry-python/pull/2054))
- `opentelemetry-api` Attribute keys must be non-empty strings.
([#2057](https://github.com/open-telemetry/opentelemetry-python/pull/2057))

Expand Down
13 changes: 8 additions & 5 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 @@ -67,7 +67,7 @@
_DEFAULT_OTEL_LINK_ATTRIBUTE_COUNT_LIMIT = 128


_ENV_VALUE_UNSET = "unset"
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We did but spec recommends using empty value as unlimited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

_ENV_VALUE_UNSET = ""

# pylint: disable=protected-access
_TRACE_SAMPLER = sampling._get_from_env_or_default()
Expand Down Expand Up @@ -533,7 +533,7 @@ class SpanLimits:
- All limit arguments are optional.
- If a limit argument is not set, the class will try to read its value from the corresponding
environment variable.
- If the environment variable is not set, the default value for the limit is used.
- If the environment variable is not set, the default value, if any, will be used.

Args:
max_attributes: Maximum number of attributes that can be added to a Span.
Expand Down Expand Up @@ -609,15 +609,18 @@ def __repr__(self):
def _from_env_if_absent(
cls, value: Optional[int], env_var: str, default: Optional[int] = None
) -> Optional[int]:
if value is cls.UNSET:
if value == cls.UNSET:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return None

err_msg = "{0} must be a non-negative integer but got {}"

# if no value is provided for the limit, try to load it from env
if value is None:
str_value = environ.get(env_var, "").strip().lower()
if not str_value:
# return default value if env var is not set
if env_var not in environ:
return default

str_value = environ.get(env_var, "").strip().lower()
if str_value == _ENV_VALUE_UNSET:
return None

Expand Down
8 changes: 4 additions & 4 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 @@ -1533,10 +1533,10 @@ def test_span_limits_code(self):
@mock.patch.dict(
"os.environ",
{
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: "unset",
OTEL_SPAN_EVENT_COUNT_LIMIT: "unset",
OTEL_SPAN_LINK_COUNT_LIMIT: "unset",
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT: "unset",
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: "",
OTEL_SPAN_EVENT_COUNT_LIMIT: "",
OTEL_SPAN_LINK_COUNT_LIMIT: "",
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT: "",
},
)
def test_span_no_limits_env(self):
Expand Down