Make setting null-valued attributes a no-op, and document that their behavior is undefined. #1706
Conversation
jkwatson
requested review from
anuraaga,
arminru,
bogdandrutu,
carlosalberto,
pavolloffay,
thisthat and
tylerbenson
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1706 +/- ##
============================================
- Coverage 85.48% 85.27% -0.22%
+ Complexity 1380 1376 -4
============================================
Files 164 164
Lines 5402 5386 -16
Branches 559 554 -5
============================================
- Hits 4618 4593 -25
- Misses 586 591 +5
- Partials 198 202 +4
Continue to review full report at Codecov.
|
| builder.setAttribute(key, value); | ||
| } | ||
| }); | ||
| attributes.forEach(builder::setAttribute); |
| attributes.setAttribute("arrayLong", (Long[]) null); | ||
| attributes.setAttribute("arrayDouble", (Double[]) null); | ||
| attributes.setAttribute("arrayBool", (Boolean[]) null); | ||
| assertThat(attributes.build().size()).isEqualTo(3); |
There was a problem hiding this comment.
should we check that we implement a no-op behavior instead of leaving it undefined?
There was a problem hiding this comment.
I went back and forth on this. I'm not really sure which we should do, since we're explicitly saying that behavior is undefined. :)
There was a problem hiding this comment.
If undefined, don't test for it. Some may consider tests as a kind of documentation.
There was a problem hiding this comment.
that was my thinking as well.
There was a problem hiding this comment.
I would argue that the specification says "undefined behavior", i.e. each implementation of the spec can do whatever they prefer, but the OTel SDK has a well-defined behavior. IMHO we should verify that we implement correctly the no-op behavior in every point that touches null attributes.
There was a problem hiding this comment.
I've added a test that validates the current behavior, with a clear message that it's not required behavior and would be fine to be changed.
There was a problem hiding this comment.
@thisthat As a C++ dev, I'd say that would be "implementation-defined" behavior, not undefined behavior 😀
…behavior is undefined.
Co-authored-by: Giovanni Liva <[email protected]>
jkwatson
force-pushed
the
null_valued_attributes
branch
from
c1147db to
2761a09
Compare
resolves #1703