-
Notifications
You must be signed in to change notification settings - Fork 896
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
Define null
as an invalid value for attributes and declare attempts to set null
as undefined behavior
#992
Define null
as an invalid value for attributes and declare attempts to set null
as undefined behavior
#992
Conversation
… to set `null` as undefined behavior
@open-telemetry/specs-approvers Please review this. There was general agreement on this on the last Spec SIG call, but we need actual approvals 😉 |
So, with this change, making any null-valued attribute setting into a no-op would be a valid implementation? That would definitely simplify some logic in the Java API/SDK. |
Yes, as long as you don't advertise that behavior. The docs should say that it is "invalid/undefined/should not" call with null. |
Approving overall - let's remove the following line and we are done:
|
I added an entry in the spec compliance matrix to keep track of implementations adopting this change in 39c6260. |
Ping @SergeyKanzhelev @tigrannajaryan - feedback has been addressed ;) |
processors / exporters. | ||
|
||
Attribute values of `null` are not valid and attempting to set a `null` value is | ||
undefined behavior. |
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.
I recommend adding a sentence that recommends SIGs to use @Nonnull
or other compile-time / lint-time indicators to prevent null if possible.
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.
Please let's do this in a follow up, as it's a) sugar on top and b) This change has been delayed forever.
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.
Sounds good to me.
@carlosalberto @tigrannajaryan @jmacd since you were opposed to the previous instruction targeted to users, would you be fine with something like that?
undefined behavior. | |
undefined behavior. | |
APIs SHOULD be annotated accordingly, if compile-time/lint-time hints are available | |
in the respective language (e.g., `@Nonnull` in Java). |
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.
Ah, I missed that comment @carlosalberto. I'm fine with merging without this but maybe we happen to find a quick agreement on that detail and can do it at once.
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.
LGTM
… to set `null` as undefined behavior (open-telemetry#992)
Resolves #797.
Closes #971.
Changes
This is an alternative proposal to #971 as requested in yesterday's SIG spec meeting.
This defines attempts to set
null
as undefined behavior and removes the current behavior that settingnull
removes any previously set attribute, which was opposed on #797.Declaring it as undefined allows for non-breaking definitions of this behavior in future. Future options include ignoring such calls with or without deleting any previously set attribute for that key, or preserving
null
as a valid value, with or without preserving its type, if any.