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.

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

Clarify boundaries of numeric env vars #4331

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Dec 9, 2024

Resolves #4283.

  • Adds new guidance indicates that for timeout variables, "implementations SHOULD interpret timeout values of zero as indefinite".
  • Clarifies range of acceptable values for OTEL_ATTRIBUTE_* / OTEL_SPAN_ATTRIBUTE_* / OTEL_LOGRECORD_ATTRIBUTE_*, OTEL_BSP_MAX_QUEUE_SIZE, OTEL_BLRP_MAX_QUEUE_SIZE.

Related PR to opentelemetry-configuration: open-telemetry/opentelemetry-configuration#151

@jack-berg jack-berg requested review from a team as code owners December 9, 2024 23:57
@reyang
Copy link
Member

reyang commented Dec 10, 2024

Need a changelog entry.

carlosalberto added a commit that referenced this pull request Dec 21, 2024
Related to #4283.

A
[comment](#4331 (comment))
adding a "type" column to each env var, but didn't feel appropriate to
extend scope of #4331.

---------

Co-authored-by: Carlos Alberto Cortez <[email protected]>
Co-authored-by: Reiley Yang <[email protected]>
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Comment on lines +129 to +131
Timeout is a sub-classification of [duration](#duration): All timeouts are also
durations, but not all durations are timeouts.

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 important? I ask because it seems that 0 duration is defined as zero milliseconds, and so it's hard for me to think of timeouts as a "subclass" of duration since they don't have that behavior

Suggested change
Timeout is a sub-classification of [duration](#duration): All timeouts are also
durations, but not all durations are timeouts.
Timeout is a sub-classification of [duration](#duration): All timeouts are also
durations, but not all durations are timeouts.

Copy link
Member Author

Choose a reason for hiding this comment

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

From duration we get:

Any value that represents a duration MUST be an integer representing a number of milliseconds. The value is non-negative - if a negative value is provided, the implementation MUST generate a warning, gracefully ignore the setting and use the default value if it is defined. For example, the value 12000 indicates 12000 milliseconds, i.e., 12 seconds.

From timeout's perspective, the useful bits which are inherited are:

  • timeouts are integers representing a number of milliseconds
  • values are non-negative, warn if negative value is given

You make a good point that the zero value semantics are different for duration and timeout. We could keep it as is, where timeout essentially overrides the duration zero value semantics. Copy the bits of duration semantics we care about into timeout.

Copy link
Member

Choose a reason for hiding this comment

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

We could keep it as is, where timeout essentially overrides the duration zero value semantics

this would make sense to me if the timeout restricted ("narrowed") the behavior of its "super type", but that's not the case here, so it doesn't seem like a duration in my mind (a timeout can't be used in places where a duration would be used). probably I'm thinking too much OOP

Comment on lines +237 to +242
| OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT | Maximum allowed attribute value size | no limit | [Integer][] | Valid values are non-negative. |
| OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT | Maximum allowed span attribute count | 128 | [Integer][] | Valid values are non-negative. |
| OTEL_SPAN_EVENT_COUNT_LIMIT | Maximum allowed span event count | 128 | [Integer][] | Valid values are non-negative. |
| OTEL_SPAN_LINK_COUNT_LIMIT | Maximum allowed span link count | 128 | [Integer][] | Valid values are non-negative. |
| OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT | Maximum allowed attribute per span event count | 128 | [Integer][] | Valid values are non-negative. |
| OTEL_LINK_ATTRIBUTE_COUNT_LIMIT | Maximum allowed attribute per span link count | 128 | [Integer][] | Valid values are non-negative. |
Copy link
Member

Choose a reason for hiding this comment

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

can we say anything useful about the 0 value for these env vars?

Copy link
Member Author

Choose a reason for hiding this comment

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

The zero value is valid here. Are you suggesting a change in semantics or improved phrasing to make it more clear that zero is valid?

Copy link
Member

Choose a reason for hiding this comment

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

ah, I wasn't sure if 0 mean none or unlimited here

but re-reading it seems that none is the obvious interpretation and may not need clarification

sort of related, it's a bit odd that the default value of OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT isn't expressible as a valid value, maybe it should allow -1 for unlimited? (might be a nice general property for declarative config to always be able to "get back" the default value when someone else has overridden it)

Copy link
Member Author

Choose a reason for hiding this comment

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

sort of related, it's a bit odd that the default value of OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT isn't expressible as a valid value, maybe it should allow -1 for unlimited

I don't disagree - but I'm reluctant to try to tackle that in this PR 😛.

A user can use effectively express the default config with something like the max value of a 32 bit integer (2,147,483,647). Obviously its a lot more cumbersome than -1.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

A couple of non-blocking comments above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify that exporter timeout settings must be positive
8 participants