Skip to content

Add an option to limit length of values of attributes and metric values#1130

Merged
carlosalberto merged 40 commits intoopen-telemetry:mainfrom
jtmalinowski:feature/span-attribute-size-limit
Aug 3, 2021
Merged

Add an option to limit length of values of attributes and metric values#1130
carlosalberto merged 40 commits intoopen-telemetry:mainfrom
jtmalinowski:feature/span-attribute-size-limit

Conversation

@jtmalinowski
Copy link
Copy Markdown
Contributor

@jtmalinowski jtmalinowski commented Oct 22, 2020 •
edited
Loading

Docs are based on existing Java implementation open-telemetry/opentelemetry-java#1484

Fixes #1133

Messaging about truncation was extracted to open-telemetry/semantic-conventions#1163

Metrics support extracted to #1830

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Oct 22, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@jtmalinowski jtmalinowski marked this pull request as ready for review October 22, 2020 13:52
@jtmalinowski jtmalinowski requested review from a team October 22, 2020 13:52
@Oberon00
Copy link
Copy Markdown
Member

Oberon00 commented Oct 22, 2020

Please create an issue for this PR.

@Oberon00 Oberon00 added area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory labels Oct 22, 2020
iNikem
iNikem reviewed Oct 22, 2020
View reviewed changes
Comment thread specification/trace/sdk.md Outdated
@jtmalinowski jtmalinowski requested a review from iNikem October 22, 2020 19:37
@jtmalinowski jtmalinowski requested a review from a team October 23, 2020 11:45
@jtmalinowski jtmalinowski changed the title Add an option to limit span attribute length Add an option to limit length of values of span attributes and metric values Oct 23, 2020
@jtmalinowski
Copy link
Copy Markdown
Contributor Author

jtmalinowski commented Oct 23, 2020

PR extended to include metric label values.

arminru
arminru reviewed Oct 23, 2020
View reviewed changes
Comment thread specification/trace/sdk.md Outdated
Comment thread specification/trace/sdk.md Outdated
Comment thread specification/sdk-environment-variables.md Outdated
Comment thread specification/trace/sdk.md Outdated
Comment thread specification/metrics/sdk.md Outdated
Comment thread specification/metrics/sdk.md Outdated
@jtmalinowski @arminru
Limits docs improvements
ed9bc5f 
Co-authored-by: Armin Ruech <[email protected]>
@Oberon00
Copy link
Copy Markdown
Member

Oberon00 commented Oct 23, 2020 •
edited
Loading

PR extended to include metric label values.

👍

I was just reminded that resource values might also be relevant here. Sorry for the piece-wise comments 🙈

@Oberon00
Copy link
Copy Markdown
Member

Oberon00 commented Oct 23, 2020

One more thing @jtmalinowski: Please either fill out the PR description or remove the part of the template that you don't use.

iNikem
iNikem reviewed Oct 23, 2020
View reviewed changes
Comment thread specification/trace/sdk.md Outdated
@jtmalinowski @iNikem
Fix traces limiting requirement
dabb292 
Co-authored-by: Nikita Salnikov-Tarnovski <[email protected]>
@jtmalinowski jtmalinowski requested a review from arminru October 26, 2020 13:23
@jtmalinowski jtmalinowski mentioned this pull request Oct 26, 2020
Closed
jmacd
jmacd reviewed Oct 27, 2020
View reviewed changes
Comment thread specification/metrics/sdk.md Outdated
Comment thread specification/sdk-environment-variables.md Outdated
@jtmalinowski jtmalinowski mentioned this pull request Jul 27, 2021
Open
@jtmalinowski
Copy link
Copy Markdown
Contributor Author

jtmalinowski commented Jul 27, 2021

@arminru @jsuereth @yurishkuro All issues addressed!

yurishkuro
yurishkuro reviewed Jul 27, 2021
View reviewed changes
Comment thread specification/common/common.md Outdated

#### Exempt Entities

Attributes which belong to Metrics MUST NOT implement the aforementioned
Copy link
Copy Markdown
Member

@yurishkuro yurishkuro Jul 27, 2021

Choose a reason for hiding this comment

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

again, you cannot put MUST's into spec if the intention is to change the rules in the near future. It's not a backwards compatible change. Either leave the behavior unspecified or use the weak MAY.

@iNikem
Copy link
Copy Markdown
Contributor

iNikem commented Jul 27, 2021

@jsuereth Do you approve or something else should be changed?

@jsuereth
Copy link
Copy Markdown
Contributor

jsuereth commented Jul 29, 2021

Sorry for delay @iNikem. Changes LGTM

@jtmalinowski
Copy link
Copy Markdown
Contributor Author

jtmalinowski commented Jul 29, 2021

@arminru Let's do it 🚀

@carlosalberto
Copy link
Copy Markdown
Contributor

carlosalberto commented Jul 29, 2021

Fine to merge but I really want to have a follow up having what happens when you define both OTEL_ATTRIBUTE_COUNT_LIMIT and related variables, e.g. OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT. At this moment it's not clear which one has higher priority.

@jtmalinowski
Copy link
Copy Markdown
Contributor Author

jtmalinowski commented Jul 30, 2021

@carlosalberto https://github.com/open-telemetry/opentelemetry-specification/pull/1130/files#diff-500cb43b9725373c25ea03660c293e59681e6e2f6ba3eb588188ed3b83dcbdcdR78 describes it like so:

An SDK MAY implement model-specific limits, for example SpanAttributeCountLimit. If both a general and a model-specific limit are implemented, then the SDK MUST first attempt to use the model-specific limit [...]

I think that it can be inferred from the context that it extends to environment variables, ie. if OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT is set or has a default then for Span Attributes it takes priority.

@iNikem
Copy link
Copy Markdown
Contributor

iNikem commented Aug 3, 2021

@open-telemetry/technical-committee can anybody press merge?

@carlosalberto
Copy link
Copy Markdown
Contributor

carlosalberto commented Aug 3, 2021

I think that it can be inferred from the context that it extends to environment variables, ie. if OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT is set or has a default then for Span Attributes it takes priority.

I think we can do better than leave things inferred and we can put a link to this section from the env vars ;) But let's definitely do that as a minor amendment after this PR.

@carlosalberto carlosalberto merged commit 7cef355 into open-telemetry:main Aug 3, 2021
@rauno56 rauno56 mentioned this pull request Aug 6, 2021
Closed
@ahayworth ahayworth mentioned this pull request May 11, 2022
Closed
This was referenced Oct 27, 2023
Closed
Closed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@jtmalinowski
Add an option to limit length of values of attributes and metric valu…
79d0057 
…es (open-telemetry#1130)
schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025
@jtmalinowski @jsuereth
Add an option to limit length of values of attributes and metric valu…
2ecdb1f 
…es (open-telemetry#1130)
schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025
@jtmalinowski @jsuereth
Add an option to limit length of values of attributes and metric valu…
43e0a08 
…es (open-telemetry#1130)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosalberto carlosalberto carlosalberto approved these changes

@cijothomas cijothomas cijothomas left review comments

@jsuereth jsuereth jsuereth approved these changes

@jmacd jmacd jmacd approved these changes

@MrAlias MrAlias MrAlias approved these changes

@arminru arminru Awaiting requested review from arminru

@reyang reyang Awaiting requested review from reyang

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

+4 more reviewers

@iNikem iNikem iNikem approved these changes

@jkwatson jkwatson jkwatson approved these changes

@obecny obecny obecny approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@arminru arminru

Labels

area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ability to limit length of values of attributes and metric values