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

Prefer global limit over model-specific default #1893

Merged

Conversation

owais
Copy link
Contributor

@owais owais commented Aug 27, 2021

Fixes #1878

Changes

Prefer global limit over model-specific limit default when model-specific limit is not set.

@owais owais requested review from a team August 27, 2021 01:54
@github-actions
Copy link

github-actions bot commented Sep 9, 2021

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 9, 2021
Copy link

@jtmal-signalfx jtmal-signalfx left a comment

Choose a reason for hiding this comment

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

I suppose the main question is who has already implemented this. It might be easy to miss by SDK implementors.

Otherwise I think that model-explicit > global-explicit > model-default > global-default sounds good. Unless more people think that in general global limits are too dangerous, and we'd rather only have the model-specific limits.

@owais
Copy link
Contributor Author

owais commented Sep 9, 2021

We implemented this in the Python SDK and most people who reviewed it were surprised by the fact that it prefers model-specific default over user specified global.

@jtmalinowski
Copy link
Contributor

Right, so when I was writing that rule I considered both cases, and went model-specific defaults, so there's a smaller chance of a user shooting themselves in the foot, by setting something globally and not understanding consequences. But good to hear that it was tested in practice, and now we now which way is preferred.

specification/common/common.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Stale label Sep 10, 2021
@owais owais requested a review from iNikem September 10, 2021 16:39
@arminru arminru added area:configuration Related to configuring the SDK area:sdk Related to the SDK labels Sep 14, 2021
@owais owais requested a review from a team September 14, 2021 09:58
@owais owais force-pushed the change-order-of-global-model-limits branch from 1f963e2 to 011141e Compare September 14, 2021 09:59
@carlosalberto carlosalberto merged commit e139901 into open-telemetry:main Sep 14, 2021
@owais owais deleted the change-order-of-global-model-limits branch September 14, 2021 11:43
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration Related to configuring the SDK area:sdk Related to the SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model specific limits should attempt to use global limits before falling back o defaults
8 participants