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

Define "none" numeric value #3000

Closed
wants to merge 1 commit into from
Closed

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Dec 2, 2022

Fixes N/A

Changes

Define "none" numeric value.

It will be useful to correctly set OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT to no limit. For now, it should use int.maxvalue or similar values.

Related issues for enum handling #2102

@Kielek Kielek requested review from a team December 2, 2022 09:03
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

👍

PS. Feels very "Pythonish" 😉

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

First, I don't think this is specific to numeric values. Booleans, strings, enums, etc., might accept "null" values just as well.
Second, there is no concept of "null value" in this environment specification, so this specification, as written now, has no meaning (or an unclear meaning).
Your goal seems to allow explicitly setting "no limit" in the limit configuration. You should add a more specific spec to that subsection, instead of the generic section here.

(PS: Congrats on creating PR #3000 🥳)

@reyang
Copy link
Member

reyang commented Dec 2, 2022

It will be useful to correctly set OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT to no limit. For now, it should use int.maxvalue or similar values.

Why would someone want "no limit"? Related to #2960 (comment).

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Need to understand why "no limit" is needed.

@pellared
Copy link
Member

pellared commented Dec 5, 2022

Need to understand why "no limit" is needed.

I agree that "unbounded limits" is a bad practice from "reliability" standpoint. But I do not see anything in the spec that disallows limitless configurations. Current default for OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT and OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT is "no limit". Wouldn't changing it be a breaking change?

Related comment: #2045 (comment)

If it is possible to have a unlimited configuration then we should allow setting it.

I have no clear nor strong opinion 😆

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Dec 13, 2022
@Kielek
Copy link
Contributor Author

Kielek commented Dec 15, 2022

I am closing this issue per last .NET AutoInstrumentation Discussion with @carlosalberto.
For now, there will be no possibility to set no-limit options. It is considered as to unpredictable.

Instead we should consider to change default values for OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT and OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT . I do not think that is is good practice to have default value which cannot be set by appropriate env. value.

@Kielek Kielek closed this Dec 15, 2022
@Kielek Kielek deleted the patch-1 branch December 15, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants