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

Finalize SpanLimits definition, add option to configure it #1416

Merged
merged 6 commits into from
Feb 10, 2021

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Feb 9, 2021

Followup from #1407 when we were asked to exclude the environment variables for the span limits.

Update:
Discussed with the @open-telemetry/technical-committee if we want to have this or not, and decision was to include this PR in the version 1.0 that will be released today:

  • This PR does not add a new functionality, the limits were defined before and defined only as configurable via ENV variables. Having the limits was and will remain optional.
  • This PR indeed does make required to add the code configuration as well and standardize it, for languages that implement this feature, but all languages which support the ENV variables have a way to configure this via code anyway, so this PR tries only to standardize on that mechanism and will be a small rename (if any).

specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
Copy link
Contributor

@tedsuo tedsuo left a comment

Choose a reason for hiding this comment

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

Since these limits are not required to be implemented, I think it is fine to add these limits now. We don't currently define what happens when these limits are surpassed. That is also fine; it should be left to the implementor to take what whatever action makes these limits effective in their language, should they need to implement them.

specification/trace/sdk.md Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member Author

@yurishkuro please approve if you think is ok, I updated to must

@bogdandrutu bogdandrutu force-pushed the spanlimits branch 2 times, most recently from 74471be to 6f520dc Compare February 10, 2021 03:20
@bogdandrutu
Copy link
Member Author

@tedsuo your comment is no longer applying based on the feedback from @yurishkuro.

specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
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.

This adds a nontrivial new MUST requirement right before release. Note that support for environment variables in SDKs is only a MAY, so SDKs are likely to not have this implemented at all.
Also, since we immediately make this stable without review period (except for this PR), our options to fix issues later are limited.

I don't understand why this seems to be so urgent. The affected env vars are marked experimental now, so this is not blocking release of 1.0, right?

@Oberon00
Copy link
Member

@tedsuo

Since these limits are not required to be implemented, I think it is fine to add these limits now.

The PR makes it a MUST to make it configurable though. Technically you could make the configuration interface a no-op I suppose, but is this really the intention here?

@yurishkuro
Copy link
Member

This adds a nontrivial new MUST requirement right before release.

Good call. I think it's better to say "if the SDK implements the limits then the limits MUST be configurable"

@carlosalberto
Copy link
Contributor

if the SDK implements the limits then the limits MUST be configurable

I like this, seems like a good trade-off.

@bogdandrutu
Copy link
Member Author

@Oberon00 please unblock the PR just in case I don't fix this until you close the day, if you are ok with @yurishkuro suggestion and I will do that.

@Oberon00
Copy link
Member

I still don't understand why we must rush this into 1.0, but since the TC seems to be in broad agreement and the MAY/MUST issue was fixed, I will do as requested and lift my request-changes.

@Oberon00 Oberon00 dismissed their stale review February 10, 2021 15:46

Dismissed on request and because main concern is fixed -- I'd still prefer to not have this in 1.0

@carlosalberto
Copy link
Contributor

@Oberon00

One factor is the desire to keep environment variables (if implemented by the SIG) as stable as possible, as they are user-facing API points. At the same time, I think there are potential improvements that can be done on top without breaking this (as #1425

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Feb 10, 2021

@Oberon00 almost all languages that are in RC or close to GA implemented this functionality in different ways, I made a search and saw that js has something, java has something else etc. That configuration was in the RC so I think is better to have it in the specs since at least 5 different languages implemented this differently.

specification/trace/sdk.md Show resolved Hide resolved
@Oberon00
Copy link
Member

Oberon00 commented Feb 10, 2021

Seeing as there are many comments / discussion is still somewhat active I think we should at least white the usual 48 h period before merging this.

EDIT: Quoting CONTRIBUTING.md:

A PR is considered to be ready to merge when:
...

  • It has been at least two working days since the last modification (except for the trivial updates, such like typo, cosmetic, rebase, etc.). This gives people reasonable time to review.

@bogdandrutu
Copy link
Member Author

@Oberon00 all the comments are about "wording" not about the meaning of the change. I think we all agreed that this is a MUST is you implement the limits which is a MAY. I don't think the discussion is controversial in any way.

@Oberon00
Copy link
Member

Oberon00 commented Feb 10, 2021

I agree the discussion is not controversial. But on the other hand, if the goal is to keep this stable, a review period of the usual length will help to have a solid wording where we don't need any "hotfixes" later. I just have a bad feeling about rushing this in so fast for 1.0.

@bogdandrutu
Copy link
Member Author

@Oberon00 I documented in the PR description the decision discussed with the TC members.

@bogdandrutu bogdandrutu merged commit b907023 into open-telemetry:main Feb 10, 2021
@bogdandrutu bogdandrutu deleted the spanlimits branch February 10, 2021 19:01
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…metry#1416)

* Finalize SpanLimits definition, add option to configure it

Signed-off-by: Bogdan Drutu <[email protected]>

* Address some of the feedback

Signed-off-by: Bogdan Drutu <[email protected]>

* Update default for limits per event and link

Signed-off-by: Bogdan Drutu <[email protected]>

* Add entry to the spec matrix

Signed-off-by: Bogdan Drutu <[email protected]>

* Fix last feedback, allow this to be a class or not

Signed-off-by: Bogdan Drutu <[email protected]>

* Mark span-limits optional in the compliance matrix

Signed-off-by: Bogdan Drutu <[email protected]>
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.