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

Exempt resources from attribute limits #1892

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

owais
Copy link
Contributor

@owais owais commented Aug 27, 2021

Resources are not susceptible to scenarios where excessive attributes
can be recorded unlike Spans. Resources are also immutable and it can be
hard for some SDKs to apply the limits at source at the time the
attributes are added to a resource. Also Limits and Resources
both are generally defined and passed on to a TracerProvider which
forces a TracerProvider to either mutate the resource or generate a new
one with duplicate attributes in order to apply the limits to it.

I propose we explicitly exempt Resources from attribute limits.

Changes

Exempt Resources from attribute limits.

@owais owais force-pushed the exempt-resource-from-limits branch from cb50353 to 0b93524 Compare August 27, 2021 01:42
@owais owais marked this pull request as ready for review August 27, 2021 01:43
@owais owais requested review from a team August 27, 2021 01:43
@owais owais force-pushed the exempt-resource-from-limits branch from 0b93524 to f560401 Compare August 27, 2021 01:44
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

From the metric perspective, resource attributes also need to be exempt from limits because they are part of the metric stream identity https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#opentelemetry-protocol-data-model

specification/common/common.md Outdated Show resolved Hide resolved
@owais owais force-pushed the exempt-resource-from-limits branch from f560401 to 44b38e5 Compare August 30, 2021 09:18
@carlosalberto
Copy link
Contributor

Ping @iNikem @open-telemetry/specs-metrics-approvers

@jsuereth
Copy link
Contributor

Related issue for metrics: #1782

@iNikem
Copy link
Contributor

iNikem commented Sep 7, 2021

Spec modification as proposed by this PR nicely described why resource attributes CAN be excluded from attribute limits. But it does not explain why they SHOULD or why do we want to have such an exception. PR description has some justification:

Also Limits and Resources both are generally defined and passed on to a TracerProvider which forces a TracerProvider to either mutate the resource or generate a new one with duplicate attributes in order to apply the limits to it

Are current attribute limits specific/scoped into TracerProvider?

@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 Sep 15, 2021
@Oberon00 Oberon00 added area:sdk Related to the SDK spec:resource Related to the specification/resource directory labels Sep 15, 2021
@owais owais force-pushed the exempt-resource-from-limits branch from 44b38e5 to 8a2aee6 Compare September 15, 2021 11:00
@owais
Copy link
Contributor Author

owais commented Sep 15, 2021

Are current attribute limits specific/scoped into TracerProvider?

@iNikem Yes, I can confirm at least Java, Python, Go and Node/JS do this.

@github-actions github-actions bot removed the Stale label Sep 16, 2021
Copy link
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

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

@owais I still think we should add some justification about "Why" from PR description into the text added

Resources are not susceptible to scenarios where excessive attributes
can be recorded unlike Spans. Resources are also immutable and it can be
hard for some SDKs to apply the limits at source at the time the
attributes are added to a resource. Furthermore, limits and Resources
both are generally defined and passed on to a TracerProvider which
forces a TracerProvider to either mutate the resource or generate a new
one with duplicate attributes in order to apply the limits to it.
@owais owais force-pushed the exempt-resource-from-limits branch from 8a2aee6 to 998ba83 Compare September 18, 2021 10:32
@owais
Copy link
Contributor Author

owais commented Sep 18, 2021

@iNikem added.

@carlosalberto
Copy link
Contributor

Ping @jsuereth

@tigrannajaryan
Copy link
Member

Reviewers, please respond to unresolved comments. If you want to block merging then request changes, otherwise I am going to merge this since it has the required number of approvals.

@tigrannajaryan
Copy link
Member

No further comments, merging.

@tigrannajaryan tigrannajaryan enabled auto-merge (squash) September 22, 2021 23:48
@tigrannajaryan tigrannajaryan merged commit 071d3c8 into open-telemetry:main Sep 22, 2021
@owais owais deleted the exempt-resource-from-limits branch September 23, 2021 12:11
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Resources are not susceptible to scenarios where excessive attributes
can be recorded unlike Spans. Resources are also immutable and it can be
hard for some SDKs to apply the limits at source at the time the
attributes are added to a resource. Furthermore, limits and Resources
both are generally defined and passed on to a TracerProvider which
forces a TracerProvider to either mutate the resource or generate a new
one with duplicate attributes in order to apply the limits to it.

Co-authored-by: Tigran Najaryan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:resource Related to the specification/resource directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants