EventPipe env var to disable stack collection #82470
Conversation
|
How does tools that expects stacks react when events turn up without stacks? Collecting a stack for a specific event is part of its manifest. This property will override that globally, so curious how that would affect tooling not getting what's currently anticipated. |
| static | ||
| inline | ||
| bool | ||
| ep_rt_config_value_get_disable_stacks (void) |
There was a problem hiding this comment.
Nit: I'd suggest we name the APIs and fields in positive rather than negative terms so we can avoid all the logic becoming double negatives. Ie: if(ep_session_get_stacks_enabled()) { // do stack stuff }
There was a problem hiding this comment.
Using an environment variable to control this seems like the wrong approach to me.
It should be a diagnostic server command/option.
Otherwise I don't see how this will work for ios/Android/wasm. /cc @lateralusX
This isn't intended to be the long term solution. I also plan on introducing a way to turn off stack collection when starting a session via the IPC, but we have teams who are impacted by the performance of stack walking that need an immediate way to turn it off. |
A well formed parser should be able to handle missing stacks. Perfview/Traceevent does for sure, it's possible that other parsers may have issues but I would consider it a bug in that parser. We already may not emit a stack for an event, there are various cases where we will bail already - if we don't have a managed thread set up, or if rundown is enabled we will skip it. |
Agree, disable it per EventPipe session over IPC will be the long term, more flexible solution. We already have the ability to pass in key value pair through dotnet-trace that should affect the providers configuration, maybe we could leverage that to turn off stack traces on a finer level of control, even using the event mask for the provider to control it on groups of events. If there is urgency to put something in place short term for desktop/server side, then current approach is ok and will not conflict with long-term IPC solution that would override the "global" setting. There is currently a handful of env variable in play in EventPipe, but I don't see any urgent need to surface this particular one to mobile until we have an IPC solution in place. |
davmason
force-pushed
the
eventpipe_stacks_8
branch
from
d4489d7
to
3e225cd
Compare
|
Fixed @davmason |
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4319033176 |
|
@davmason backporting to release/7.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Env var to disable stack collection
Using index info to reconstruct a base tree...
M src/coreclr/inc/clrconfigvalues.h
M src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h
M src/mono/mono/eventpipe/ep-rt-mono.h
M src/native/eventpipe/ep-buffer-manager.c
M src/native/eventpipe/ep-rt.h
M src/native/eventpipe/ep-session.c
Falling back to patching base and 3-way merge...
Auto-merging src/native/eventpipe/ep-session.c
Auto-merging src/native/eventpipe/ep-rt.h
CONFLICT (content): Merge conflict in src/native/eventpipe/ep-rt.h
Auto-merging src/native/eventpipe/ep-buffer-manager.c
Auto-merging src/mono/mono/eventpipe/ep-rt-mono.h
Auto-merging src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h
CONFLICT (content): Merge conflict in src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h
Auto-merging src/coreclr/inc/clrconfigvalues.h
CONFLICT (content): Merge conflict in src/coreclr/inc/clrconfigvalues.h
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Env var to disable stack collection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@davmason an error occurred while backporting to release/7.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
ghost
locked as resolved and limited conversation to collaborators
Collecting stacks can often be extremely expensive, so we should let customers who don't care about stacks opt out. Longer term I would like to have a better story but this is a much needed stopgap measure.