Follow spec on span limits, batch processors#7030
Follow spec on span limits, batch processors#7030jack-berg merged 3 commits intoopen-telemetry:mainfrom
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. |
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
This looks good to me. I had one small comment about a test, but looks fine. Thanks for the contribution!
| @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.