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

Change various usages of minimum to exclusiveMinimum #151

Merged
merged 4 commits into from
Jan 15, 2025

Conversation

jack-berg
Copy link
Member

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

Related to open-telemetry/opentelemetry-specification#4283, but not blocked by the resolution over there.

With #142, the main principle of what constitutes an allowed change is whether it makes validation more or less restrictive. In reviewing our use of the JSON schema minimum keyword, which has inclusive semantics, I found a variety of areas which may come back to bite us. In all these cases, the minimum is currently set to 0. But if the user sets the property value to zero, the semantics are unclear.

Best to prohibit zero in these cases, since we can later loosen the restriction to allow zero but won't be able to make it more strict.

Cases:

  • Processor (log, span) batch size
  • Processor (log, span) queue size
    Processor (log, span, periodic metric reader) exporter timeout
    Processor (log, span, periodic metric reader) export internal
    Exporter (otlp, zipkin) timeout
    Jaeger remote sampler fetch interval

UPDATE (1/13/2025): Updated to reflect the state of things in Spec PR#4331 which now seems poised to merge. Differences from what I originally proposed:

  • Durations can be zero.
  • Timeouts can be zero, with zero indicating indefinite.

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

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM

@jack-berg
Copy link
Member Author

Status update: waiting to see how open-telemetry/opentelemetry-specification#4331 shakes out. Based on this conversation, it appears the preference is to interpret a timeout value of zero to mean indefinite. Although not idea if given a fresh start, its a reasonable conclusion to reach given diverging implementations.

We have somewhat of a fresh start in declarative config, but are tied back to the env vars config schema via env var substitution.

@jack-berg jack-berg requested a review from marcalff January 13, 2025 16:45
@jack-berg
Copy link
Member Author

jack-berg commented Jan 13, 2025

UPDATE (1/13/2025): Updated to reflect the state of things in Spec PR#4331 which now seems poised to merge. Differences from what I originally proposed:

  • Durations can be zero.
  • Timeouts can be zero, with zero indicating indefinite.

Given these changes, I've cleared the approvals. Please take a look @open-telemetry/configuration-approvers.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, my only comment is editorial.

Lines like:

        # Configure maximum allowed time (in milliseconds) to export data. Value must be non-negative. A value of 0 indicates indefinite.

are very long in the yaml file itself.

Could type_description.yaml break this into separate lines, by adding an empty line as delimiter ?

The default configuration in examples will be copied and pasted a lot when people will use this, so a reasonable max size for a line will make it easier to consume.

@jack-berg
Copy link
Member Author

Could type_description.yaml break this into separate lines, by adding an empty line as delimiter ?

👍 yes I can do this

jack-berg added a commit to open-telemetry/opentelemetry-specification that referenced this pull request Jan 15, 2025
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

---------

Co-authored-by: Reiley Yang <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
@jack-berg jack-berg merged commit 5901c77 into open-telemetry:main Jan 15, 2025
11 checks passed
@jack-berg jack-berg mentioned this pull request Jan 15, 2025
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.

4 participants