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. |
| @Test | ||
| void invalidLogLimits() { | ||
| 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).
| @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?
3 participants
Addresses the issue mentioned #7024.