Add new configuration option to limit the size of string attribute values #1484
Conversation
iNikem
requested review from
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
pavolloffay,
thisthat and
tylerbenson
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1484 +/- ##
============================================
- Coverage 92.50% 92.33% -0.17%
- Complexity 938 951 +13
============================================
Files 117 117
Lines 3348 3380 +32
Branches 270 281 +11
============================================
+ Hits 3097 3121 +24
- Misses 170 172 +2
- Partials 81 87 +6
Continue to review full report at Codecov.
|
sdk/src/main/java/io/opentelemetry/sdk/trace/config/TraceConfig.java
Outdated
Show resolved
Hide resolved
| @@ -89,6 +93,7 @@ | |||
| private static final int DEFAULT_SPAN_MAX_NUM_LINKS = 32; | |||
| private static final int DEFAULT_SPAN_MAX_NUM_ATTRIBUTES_PER_EVENT = 32; | |||
| private static final int DEFAULT_SPAN_MAX_NUM_ATTRIBUTES_PER_LINK = 32; | |||
| private static final int DEFAULT_KEY_SPAN_ATTRIBUTE_MAX_VALUE_LENGTH = 0; | |||
There was a problem hiding this comment.
Is zero a good default? shouldn't it be MAX_INT?
There was a problem hiding this comment.
even better, make the default be a null Integer and get rid of this magic number.
There was a problem hiding this comment.
All other methods here return int. Do you want for this one to return Integer?
With default value being MAX_INT the code will look like if(traceConfig.getMaxLengthOfAttributeValues() < Integer.MAX_INT) truncate(), which is strange for me.
There was a problem hiding this comment.
I also agree that 0 is strange as a default value. I would set it to a more common magic number, e.g. 255.
There was a problem hiding this comment.
How about if(traceConfig.getMaxLengthOfAttributeValues() != TraceConfig.DEFAULT_MAX_ATTRIBUTE_LENGTH) ? I think that reads great, makes it clear, and doesn't use '0' as a magic number.
There was a problem hiding this comment.
I think I prefer MAX_VALUE, to the nullable, but I don't feel super strongly about it.
There was a problem hiding this comment.
Wait, that does not make sense. Currently I attempt to truncate values if configured value differs from "unlimited" which is signalled by 0. Your proposal says "let's try to truncate if configured value is not default". But what if we change default to, say, 2M? if(traceConfig.getMaxLengthOfAttributeValues() != TraceConfig.DEFAULT_MAX_ATTRIBUTE_LENGTH) will not work anymore. Compare with default is wrong, default may change. We have to compare with "unlimited", however we denote that. And I like traceConfig.getMaxLengthOfAttributeValues() > 0 more than traceConfig.getMaxLengthOfAttributeValues() > Integer.MAX_INT.
There was a problem hiding this comment.
I'd be happier with -1, rather than 0. 0 is actually a valid length, and -1 clearly is not. And, I'd rather have a constant defined for the "unset" value, rather than just hardcode the magic number in two places.
There was a problem hiding this comment.
👍 I like -1 for unlimited, I think that's reasonably conventional
|
Should I do any performance testing? If yes, then how? Do we have some testbed/benchmark? |
There are some benchmarks in the sdk/src/jmh directory, but I'm not sure if any of them would be relevant. Please add something there that would exercise this, if you have time. |
|
I have add a benchmark, see Unsurprisingly, truncating strings takes time. Truncating long strings to long strings takes a lot of time. |
|
looks like some checkstyle issues to be resolved still |
|
Just curious...what JVM version did you run the benchmarks on? Do they change much with a different version? |
|
Docs are based on existing Java implenetation open-telemetry/opentelemetry-java#1484
Closes #1478
I have implemented a simpler version for now, allowing to specify limits in characters, not bytes. I think it is not ideal, but probably an acceptable compromise between precision and complexity.