-
Notifications
You must be signed in to change notification settings - Fork 431
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
Add Feature Flag to Disable Array Preallocation for Log Record Attributes in Limited Stack Environments #2055
Add Feature Flag to Disable Array Preallocation for Log Record Attributes in Limited Stack Environments #2055
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2055 +/- ##
=======================================
+ Coverage 77.3% 77.9% +0.5%
=======================================
Files 124 121 -3
Lines 21282 21136 -146
=======================================
- Hits 16472 16471 -1
+ Misses 4810 4665 -145 ☔ View full report in Codecov by Sentry. |
opentelemetry-sdk/Cargo.toml
Outdated
@@ -51,6 +51,7 @@ testing = ["opentelemetry/testing", "trace", "metrics", "logs", "rt-async-std", | |||
rt-tokio = ["tokio", "tokio-stream"] | |||
rt-tokio-current-thread = ["tokio", "tokio-stream"] | |||
rt-async-std = ["async-std"] | |||
no-preallocate-attributes-array = [] |
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.
Alternate suggestion:
disable_stack_log_attributes
"Preallocate" is potentially misleading, we can pre-allocate in heap as well.
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.
please also add a changelog entry for this. There is a general lacking of documentaion for each of the feature flags, but that is something we need to work on independent of this PR.
Do we have users who have tried to run OpenTelemetry Rust in Kernel mode? I'm asking mainly to understand if the lack of this feature is an actual blocker for anyone. |
We don't yet support kernel mode, because of usage of |
Is it better to delay adding this feature flag until we reach that state? Just to keep overall feature-flags under control. |
I would be in favor of that.
It'd be good to try it out first and check if we run into issues. (once we start supporting this scenario). We allocate 10 instances of |
sure, I actually prefer that. The feature flag requirement was added in #1920, so created the PR :)
I think tracing needs to be conservative for any stack allocation in kernel mode, specifically these pre-allocations in stack. Thinking about the deep call chains, and recursion in actual kernel mode application, the stack size can grow fast. But, there would be other places we need to look too, and good to revisit once we think about kernel-mode :) |
closing as discussed above. |
Fixes #1920
Changes
This PR introduces a feature flag,
no-preallocate-attributes-array
, that disables the default behavior of preallocating a fixed-size array for storing log record attributes. Instead of using a preallocated array, this feature flag enables the use of the vector part ofGrowableArray
, which allocates memory dynamically when the first entry is added.Rationale: Kernel mode and other environments with limited stack space require careful management of stack allocations. By using a dynamically growing vector instead of preallocating a fixed-size array, we can reduce the stack footprint.
Note - We could have directly used the
Vector
instead ofGrowableArray
for this scenario, however didn't find any visible difference in stress tests with either approach, so keptGrowableArray
to be consistent.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes