Follow spec on span limits, batch processors #7030
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7030 +/- ##
============================================
- Coverage 89.99% 89.96% -0.03%
- Complexity 6596 6664 +68
============================================
Files 729 748 +19
Lines 19858 20088 +230
Branches 1955 1970 +15
============================================
+ Hits 17871 18073 +202
- Misses 1389 1423 +34
+ Partials 598 592 -6 ☔ View full report in Codecov by Sentry. |
| assertThatThrownBy(() -> LogLimits.builder().setMaxNumberOfAttributes(0)) | ||
| assertThatThrownBy(() -> LogLimits.builder().setMaxNumberOfAttributes(-1)) |
There was a problem hiding this comment.
Oops, the -1 now appears in here twice. Might be nice to add a test case that covers the fact that 0 is now valid (no exception thrown).
| @@ -46,23 +46,23 @@ void updateSpanLimits_All() { | |||
|
|
|||
| @Test | |||
| void invalidSpanLimits() { | |||
| assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfAttributes(0)) | |||
| assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfAttributes(-1)) | |||
There was a problem hiding this comment.
Looks like each of these test cases now duplicates a test case directly below it. Same comment as this.
| */ | ||
| public BatchSpanProcessor build() { | ||
| checkArgument( | ||
| maxExportBatchSize <= maxQueueSize, | ||
| "maxExportBatchSize must be smaller or equal to maxQueueSize."); |
There was a problem hiding this comment.
Let's omit this for the time being, given @breedx-splk's comment here:
Why is this a requirement? Wouldn't it be ok to fill up an export batch with multiple queue drains?
Addresses the issue mentioned #7024.