-
Notifications
You must be signed in to change notification settings - Fork 901
Clarify that AttributesBuilder.put allows nulls #7271
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
Conversation
The underlying implementation already does a null check on both key and value. I think it's useful to publicly allow nulls on values since they are often null and it's helpful if callers know they don't need to null check. (I could see an argument to not allow nulls for keys, so I didn't add Nullable there.)
@@ -35,8 +36,8 @@ public interface AttributesBuilder { | |||
// version. | |||
<T> AttributesBuilder put(AttributeKey<Long> key, int value); | |||
|
|||
/** Puts a {@link AttributeKey} with associated value into this. */ | |||
<T> AttributesBuilder put(AttributeKey<T> key, T value); | |||
/** Puts an {@link AttributeKey} with an associated value into this if the value is non-null. */ |
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 might even think an additional comment is useful -- something which conveys that any existing value for that key will remain if null is passed (a null value doesn't remove/unset any current value).
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.
done.
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 like it.
You gotta make the implementations match, though. |
This would resolve #4336, but see @jkwatson's comment:
My read of this is that settings a |
That's cool, but that wouldn't prevent/preclude an implementation from clarifying what the behavior is though, would it? |
I think its debatable, but my take is that it would not prevent us from clarifying. |
I don't think we could change the behavior at this point without it being seen as a breaking change, so may as well document it |
I don't have any strong opposition to this, since it appears the spec will probably never clarify and we might as well just tell people what the current behavior is. |
api/all/src/main/java/io/opentelemetry/api/common/AttributesBuilder.java
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7271 +/- ##
=========================================
Coverage 89.61% 89.61%
+ Complexity 6863 6859 -4
=========================================
Files 780 780
Lines 20731 20718 -13
Branches 2023 2020 -3
=========================================
- Hits 18578 18567 -11
+ Misses 1514 1512 -2
Partials 639 639 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The underlying implementation already does a null check on both key and value.
I think it's useful to publicly allow nulls on values since they are often null and it's helpful if callers know they don't need to null check.
(I could see an argument to not allow nulls for keys, so I didn't add Nullable there.)
Fixes #4336