-
Notifications
You must be signed in to change notification settings - Fork 500
[TRACE SDK] Batch span processor options now using env variables #3661
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
base: main
Are you sure you want to change the base?
[TRACE SDK] Batch span processor options now using env variables #3661
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3661 +/- ##
==========================================
+ Coverage 90.11% 90.18% +0.07%
==========================================
Files 221 222 +1
Lines 7135 7154 +19
==========================================
+ Hits 6429 6451 +22
+ Misses 706 703 -3
🚀 New features to boost your workflow:
|
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.
Some changes needed:
The header file exposes implementation details, which are compiled into the application binary.
Move every implementation details, including default values, helpers, names of environment variables, to batch_span_processor_options.cc
See existing options structures for the pattern.
@marcalff Applied the above changes, Thanks! But the BatchSpanProcessor does not have the field of |
sdk/include/opentelemetry/sdk/trace/batch_span_processor_options.h
Outdated
Show resolved
Hide resolved
sdk/include/opentelemetry/sdk/trace/batch_span_processor_options.h
Outdated
Show resolved
Hide resolved
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.
Looking much better.
See minor cleanup,
I will approve once I can verify the changes.
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.
LGTM, thanks for the fix.
Fixes #2085
Using this change we can get
max_queue_size
schedule_delay
export_timeout
max_export_batch_size
using environment variables. BatchSpanProcessor still does not useexport_timeout
and tests have been added.Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes