-
Notifications
You must be signed in to change notification settings - Fork 516
[SDK] Fix BatchSpanProcessorOptions and BatchLogRecordProcessorOptions to restore C++20 aggregate initialization support #3780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…on support - Remove user-defined constructor that prevented C++20 designated initializers - Add inline helper functions in batch_span_processor_options_env namespace - Use default member initializers calling the helper functions - Update tests with corrected default values (2048 for max_queue_size, 30000ms for export_timeout) - Add test for aggregate initialization and C++20 designated initializers Co-authored-by: lalitb <[email protected]>
Co-authored-by: lalitb <[email protected]>
…gnore Co-authored-by: lalitb <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3780 +/- ##
==========================================
+ Coverage 89.91% 89.93% +0.02%
==========================================
Files 225 225
Lines 7165 7163 -2
==========================================
- Hits 6442 6441 -1
+ Misses 723 722 -1
🚀 New features to boost your workflow:
|
|
@copilot, I suggest these commits to be squashed to avoid storing accidentally committed junk like forever. |
1 similar comment
|
@copilot, I suggest these commits to be squashed to avoid storing accidentally committed junk like forever. |
I'm unable to squash commits directly since that requires force pushing, which I don't have access to. However, this can be handled when merging by using GitHub's "Squash and merge" option, which will combine all commits into a single clean commit. |
|
@copilot - please fix these: 23 | inline constexpr const char *kMaxQueueSizeEnv = "OTEL_BSP_MAX_QUEUE_SIZE"; do not use inline variables in C++14. |
Move constants inside inline functions to avoid using inline variables which are a C++17 feature not available in C++14. Co-authored-by: lalitb <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a regression in BatchSpanProcessorOptions where a user-defined constructor broke C++20 designated initializers (aggregate initialization). The solution removes the constructor and uses default member initializers with inline helper functions to preserve environment variable reading behavior while restoring aggregate type status.
Key changes:
- Refactored
BatchSpanProcessorOptionsto be an aggregate type compatible with C++20 designated initializers - Moved environment variable reading logic to inline functions in a new
batch_span_processor_options_envnamespace - Corrected default values:
max_queue_size(2084→2048) andexport_timeout(3000ms→30000ms) to align with OpenTelemetry spec
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
sdk/include/opentelemetry/sdk/trace/batch_span_processor_options.h |
Removed constructor, added inline helper functions in batch_span_processor_options_env namespace, converted to aggregate type with default member initializers |
sdk/src/trace/batch_span_processor_options.cc |
Emptied file (kept for ABI compatibility) with explanatory comment |
sdk/test/trace/batch_span_processor_test.cc |
Updated default value tests (2048, 30000ms), added new tests for aggregate initialization and C++20 designated initializers |
.gitignore |
Added CodeQL build artifacts (unrelated to main PR purpose) |
sdk/include/opentelemetry/sdk/trace/batch_span_processor_options.h
Outdated
Show resolved
Hide resolved
|
See comments in #3778 (comment) This change has implications for the single dll, and for file configuration. |
|
@copilot - the changes in this PR seems to be breaking the windows with single dll. Please do the changes as suggested below: estore aggregate initialization by:
Files to modify:
|
- Move env helper function implementations from header to .cc files - Declare helper functions with OPENTELEMETRY_EXPORT in header - Apply the same pattern to BatchLogRecordProcessorOptions - Add comprehensive documentation to both options structs - Fix "to to" typo in BatchSpanProcessorOptions documentation This fixes the Windows single DLL build issue by avoiding inline functions in header files that are called from default member initializers. Co-authored-by: lalitb <[email protected]>
... Fixed in e069099. Moved helper function implementations to |
Changes
The user-defined constructor in
BatchSpanProcessorOptionsmade it a non-aggregate type, breaking C++20 designated initializers:Solution: Remove the constructor and use default member initializers with helper functions declared in header and defined in
.ccfiles:batch_span_processor_options_env) declared withOPENTELEMETRY_EXPORTin header and defined in.ccfile (fixes Windows single DLL build)max_queue_size2084→2048,export_timeout3000ms→30000ms (aligns with OTel spec)BatchLogRecordProcessorOptionsfor consistencyFor significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changesOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.