-
Notifications
You must be signed in to change notification settings - Fork 888
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
Conversation
9402afe
to
617e5df
Compare
617e5df
to
1f57b50
Compare
There was a problem hiding this 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.
c11fd4f
to
0605705
Compare
@yurishkuro please approve if you think is ok, I updated to must |
74471be
to
6f520dc
Compare
@tedsuo your comment is no longer applying based on the feedback from @yurishkuro. |
There was a problem hiding this 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?
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? |
Good call. I think it's better to say "if the SDK implements the limits then the limits MUST be configurable" |
I like this, seems like a good trade-off. |
@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. |
377938e
to
1a9e904
Compare
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. |
Dismissed on request and because main concern is fixed -- I'd still prefer to not have this in 1.0
@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. |
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:
|
1a9e904
to
84bf08d
Compare
@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. |
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. |
c1e3634
to
7503ba7
Compare
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
7503ba7
to
974a3fb
Compare
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
974a3fb
to
05c9f5a
Compare
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@Oberon00 I documented in the PR description the decision discussed with the TC members. |
…metry#1416) * Finalize SpanLimits definition, add option to configure it Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Address some of the feedback Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Update default for limits per event and link Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Add entry to the spec matrix Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Fix last feedback, allow this to be a class or not Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Mark span-limits optional in the compliance matrix Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
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: