这是indexloc提供的服务,不要输入任何密码
Skip to content

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

Merged
merged 3 commits into from
Apr 16, 2025

Conversation

tylerbenson
Copy link
Member

@tylerbenson tylerbenson commented Apr 11, 2025

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

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.)
@tylerbenson tylerbenson requested a review from a team as a code owner April 11, 2025 21:12
@@ -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. */
Copy link
Contributor

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

I like it.

@breedx-splk
Copy link
Contributor

You gotta make the implementations match, though.

@jack-berg
Copy link
Member

This would resolve #4336, but see @jkwatson's comment:

Unfortunately, we're currently adhering strictly to the spec:

Attribute values of null are not valid and attempting to set a null value is undefined behavior.

My read of this is that settings a null attribute value is undefined at the spec level, but that doesn't necessarily prevent language implementation from defining the behavior based on what is idiomatic. I suspect that the spec language was coming from this from the perspective of null isn't idiomatic or a consistent concept across languages, and so taking a stance on it at the spec level wouldn't be appropriate.

@breedx-splk
Copy link
Contributor

My read of this is that settings a null attribute value is undefined at the spec level, but that doesn't necessarily prevent language implementation from defining the behavior based on what is idiomatic. I suspect that the spec language was coming from this from the perspective of null isn't idiomatic or a consistent concept across languages, and so taking a stance on it at the spec level wouldn't be appropriate.

That's cool, but that wouldn't prevent/preclude an implementation from clarifying what the behavior is though, would it?

@jack-berg
Copy link
Member

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.

@trask
Copy link
Member

trask commented Apr 11, 2025

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

@jkwatson
Copy link
Contributor

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.

Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a01bf6c) to head (ee2a789).
Report is 4 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jack-berg jack-berg merged commit d13f04d into open-telemetry:main Apr 16, 2025
28 checks passed
@tylerbenson tylerbenson deleted the patch-1 branch April 17, 2025 15:33
bidetofevil added a commit to bidetofevil/opentelemetry-java that referenced this pull request May 20, 2025
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.

Officially treat @Nullable value in AttributesBuilder.put() as a no-op
5 participants