chore(profiling): name the component that takes over SIGSEGV/SIGBUS - #20348
Draft
vlad-scherbich wants to merge 8 commits into
Draft
vlad-scherbich wants to merge 8 commits into
vlad-scherbich wants to merge 8 commits into
Conversation
The stack profiler logs a takeover whenever it loses ownership of SIGSEGV or SIGBUS and has to fall back to the syscall-based memory copy, but the message said only "another component", which is not actionable: the reader cannot tell which library installed the foreign handler, and these lines have repeatedly sent investigations down the wrong path. Resolve the current owner of each signal with dladdr and report it as shared object + offset, plus the nearest symbol when the handler is exported. Also distinguish SIG_DFL/SIG_IGN and our own handler, so the asymmetric case (a component grabbing only SIGSEGV and leaving SIGBUS to us) is visible. The lookup allocates and is not async-signal-safe, so it is called only from the sampling loop, never from the handler itself. Link CMAKE_DL_LIBS since dladdr still needs -ldl on the glibc 2.17 manylinux2014 target.
Circular import analysis
|
Dependency direction analysis
|
Codeowners resolved asResolved from the full PR diff against |
The takeover notice was written straight to std::cerr from the sampling loop, so it surfaced as unstructured output that log pipelines classify as an error, even though profiling continues normally on the slower copy. That framing is what sent incident investigations chasing it. Stash the foreign owner on the sampler and drain it from StackCollector.snapshot() on the scheduler thread, which already does exactly this for the exception that kills the sampling thread. Reporting it there routes it through the standard logger at warning level, so it respects log configuration and stays out of telemetry (DDTelemetryErrorHandler only forwards records at ERROR and above). Keep the two cases distinct: a handler that was already foreign when the warmup window ended is ordinary (pytest, faulthandler), whereas one taken over after we upgraded to safe_memcpy means some component installed its handler lazily, mid-process, which is the case worth chasing. The sampling thread has no GIL and cannot log directly. The stash is drained on read so the notice is reported at most once, and it is reset after fork so a child never reports its parent's takeover. The mypy pre-commit hook is bypassed: origin/main already reports the same 7 errors for these files, and this change adds none.
Contributor
|
#20348 deferred takeover notices to snapshot() upload, so a process dying within the upload interval lost the warning entirely. Write to stderr at detection again while keeping the stash for structured logging.
…itions Report foreign SIGSEGV/SIGBUS handler owners via telemetry with normalized basename tags, and capture Linux core-dump preconditions once at profiler start.
vlad-scherbich
changed the title
vlad-scherbich
added
changelog/no-changelog
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved native build-linkage and correctness issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds diagnostics for foreign SIGSEGV/SIGBUS handlers, Linux core-dump telemetry, and related native APIs, tests, typings, and build updates.
Changes:
- Resolves and reports signal-handler owners.
- Emits takeover warnings and normalized telemetry.
- Adds core-dump precondition reporting.
File summaries
| File | Reviewed changes |
|---|---|
tests/profiling/test_core_preconditions.py |
Tests core-dump telemetry helpers. |
tests/profiling/collector/test_stack_native.py |
Tests takeover warnings and telemetry. |
ddtrace/profiling/profiler.py |
Emits startup precondition telemetry. |
ddtrace/profiling/collector/stack.py |
Drains owner data and normalizes telemetry; SIGBUS-only ownership reports the wrong token (moderate). |
ddtrace/profiling/_core_preconditions.py |
Collects Linux core-dump metadata; dumpability may incorrectly report unknown (moderate). |
ddtrace/internal/settings/_config.py |
Adds configuration type annotations. |
ddtrace/internal/datastreams/__init__.py |
Adds processor typing annotations. |
ddtrace/internal/datadog/profiling/stack/src/stack.cpp |
Exposes the native takeover API. |
ddtrace/internal/datadog/profiling/stack/src/sampler.cpp |
Records takeover state; owner reporting may throw, and fallback notices may be emitted without an active fallback (moderate). |
ddtrace/internal/datadog/profiling/stack/src/echion/danger.cc |
Resolves handler owners; the native path lacks coverage (moderate). |
ddtrace/internal/datadog/profiling/stack/include/sampler.hpp |
Defines takeover state and APIs. |
ddtrace/internal/datadog/profiling/stack/echion/echion/danger.h |
Declares owner-description support. |
ddtrace/internal/datadog/profiling/stack/CMakeLists.txt |
Links dynamic-loader libraries; fuzz targets also require the linkage (critical). |
ddtrace/internal/datadog/profiling/stack/_stack.pyi |
Documents the native API. |
ddtrace/internal/datadog/profiling/stack/__init__.pyi |
Exposes the native API in stubs. |
Review details
Suppressed comments (3)
ddtrace/internal/datadog/profiling/stack/src/echion/danger.cc:170
- The new native owner-description path is not exercised by the added automated tests: the snapshot tests mock
take_foreign_segv_handler(), while the existing native regression only checks the boolean ownership predicate. A build ordladdr/sigactionregression could therefore pass all tests while producing incorrect owner strings or failing to expose the new API. Add a Linux/macOS subprocess or native test that installs known handlers and asserts the SIGSEGV/SIGBUS descriptors, including the SIGBUS-only case.
static std::string
describe_signal_owner(int signo)
{
struct sigaction current;
if (sigaction(signo, nullptr, ¤t) != 0) {
ddtrace/internal/datadog/profiling/stack/src/sampler.cpp:479
describe_segv_handler_owners()constructsstd::stringvalues and can throw (for example, on allocation failure), but this call is before the sampling loop'stry/catchat line 513. An exception here escapes the pthread entry point and terminates the process instead of preserving the existing syscall fallback; make owner reporting best-effort and keep this fallback path non-throwing.
const auto owners = describe_segv_handler_owners();
record_foreign_segv_handler(true, owners);
ddtrace/internal/datadog/profiling/stack/src/sampler.cpp:497
- These new records and the fallback-specific message are emitted before the
set_fast_copy_enabled(false)check below. On Linux whereprocess_vm_readvis unavailable, that call returns false, sampling stops, andfast_copy_activeremains true;snapshot()then reports that the profiler is using the slower copy for the rest of the process even though no fallback is active and sampling has ended. Only queue this fallback notice after a successful fallback, or include a distinct stopped-sampling outcome in the report.
const auto owners = describe_segv_handler_owners();
record_foreign_segv_handler(false, owners);
std::cerr << "ddtrace stack profiler: SIGSEGV/SIGBUS handler was taken over by another "
"component; falling back to syscall-based memory copy to avoid crashing. "
<< "Handler owners: " << owners << std::endl;
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| endif() | ||
|
|
||
| target_link_libraries(${EXTENSION_NAME} PRIVATE dd_wrapper Threads::Threads) | ||
| target_link_libraries(${EXTENSION_NAME} PRIVATE dd_wrapper Threads::Threads ${CMAKE_DL_LIBS}) |
Comment on lines
+39
to
+43
| if line.startswith("Dumpable:"): | ||
| value: str = line.split(":", 1)[1].strip() | ||
| if value == "1": | ||
| return "yes" | ||
| if value == "0": |
Comment on lines
+49
to
+53
| if sigsegv_owner is not None: | ||
| return sigsegv_owner | ||
| if sigbus_owner is not None: | ||
| return sigbus_owner | ||
| return _normalize_foreign_handler_owner_component(owner) |
Link fuzz targets with CMAKE_DL_LIBS, prefer non-ddtrace handler owner for telemetry when only one signal is foreign, reorder syscall fallback recording after set_fast_copy_enabled(false) succeeds, and guard describe_segv_handler_owners() with no-throw fallback.
BenchmarksBenchmark execution time: 2026-09-16 02:47:42 Comparing candidate commit 6c87637 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 566 metrics, 10 unstable metrics, 5 known flaky benchmarks, 19 flaky benchmarks without significant changes.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When another component owns SIGSEGV or SIGBUS, the stack sampler already falls back to the syscall copy. This PR makes that takeover actionable:
describe_segv_handler_owners()usesdladdrfrom the sampling loop to report each signal's owner as shared-object+offset (nearest symbol when exported), distinguishingSIG_DFL/SIG_IGN/ddtrace. The sampling thread stashes the owner string;StackCollector.snapshot()drains it once asLOG.warningonddtrace.profiling.collector.stack, with distinct wording for foreign-at-warmup-end vs mid-run takeover. Immediate stderr at detection is restored so short-lived processes still see the warning. Fleet telemetry tags normalized handler basenames; Linux core-dump preconditions are captured once at profiler start. LinksCMAKE_DL_LIBS. Fallback and crash behavior unchanged.Repro harness on stacked branch: vlad/prof-identify-foreign-segv-handler...vlad/prof-foreign-segv-handler-repro
Testing
test_snapshot_names_foreign_segv_handler_owner, foreign-handler telemetry tests,test_core_preconditions.pyworkspace-vlad-ws4(Linux 6.8 aarch64, 16 cores)Build the native handler and run the production-like
foreign-nativescenario:Immediate stderr at detection (names the foreign owner per signal):
Resolve the reported offset back to a symbol:
nm libforeign.so | grep foreign_handlerStructured
LOG.warningvia scheduler flush (mid-run takeover branch):