Ignore null or blank string-valued span attributes. #771
Conversation
jkwatson
requested review from
arminru,
bogdandrutu,
carlosalberto,
pavolloffay,
songy23 and
tylerbenson
as code owners
Codecov Report
@@ Coverage Diff @@
## master #771 +/- ##
===========================================
+ Coverage 78.77% 78.97% +0.2%
- Complexity 760 769 +9
===========================================
Files 98 98
Lines 2709 2711 +2
Branches 255 257 +2
===========================================
+ Hits 2134 2141 +7
+ Misses 474 470 -4
+ Partials 101 100 -1
Continue to review full report at Codecov.
|
|
What about setting an attribute on a started span, not the span builder? |
|
great catch. will update the RERS as well. |
|
@tylerbenson ok, got the active span as well |
| @@ -137,6 +138,9 @@ | |||
|
|
|||
| @Override | |||
| public Span.Builder setAttribute(String key, String value) { | |||
| if (StringUtils.isNullOrBlank(value)) { | |||
There was a problem hiding this comment.
Do we want to count this as dropped or we simply ignore?
There was a problem hiding this comment.
it's definitely a different kind of "dropping" than the "too many attributes" kind. I think ignoring it is fine..it's not really an attribute at all without a value.
There was a problem hiding this comment.
Fine with this for the moment, may be unexpected for users especially for empty.
| @@ -137,6 +138,9 @@ | |||
|
|
|||
| @Override | |||
| public Span.Builder setAttribute(String key, String value) { | |||
| if (StringUtils.isNullOrBlank(value)) { | |||
| return this; | |||
| } | |||
| return setAttribute(key, AttributeValue.stringAttributeValue(value)); | |||
There was a problem hiding this comment.
What is the behavior for AttributeValue.stringAttributeValue(value) in case a null string is passed?
There was a problem hiding this comment.
that remains the same. If you're trying to create an AttributeValue with a null, you'll get the usual NPE.
There was a problem hiding this comment.
I think this is unexpected result and inconsistent, maybe we should try to see how we can offer the same behavior.
There was a problem hiding this comment.
that's a fair comment. I think we can do this by allowing null/empty in the AttributeValue, and then ignoring when we try to add. I'll change this PR to do it that way.
| @@ -137,6 +138,9 @@ | |||
|
|
|||
| @Override | |||
| public Span.Builder setAttribute(String key, String value) { | |||
| if (StringUtils.isNullOrBlank(value)) { | |||
There was a problem hiding this comment.
Fine with this for the moment, may be unexpected for users especially for empty.
resolves #765