Skip to content

feat(crashtracker): orchestrion auto-injection aspect#5027

Draft
kakkoyun wants to merge 3 commits into
feat/crashtracker-apifrom
feat/crashtracker-orchestrion
Draft

feat(crashtracker): orchestrion auto-injection aspect#5027
kakkoyun wants to merge 3 commits into
feat/crashtracker-apifrom
feat/crashtracker-orchestrion

Conversation

@kakkoyun

Copy link
Copy Markdown
Member

Summary

Stacked on #[PR1]. Adds an orchestrion aspect that automatically prepends crashtracker.Start() as the first statement of every program's main() function at compile time, enabling transparent crash report capture without source changes.

Design choice: prepend-statements over inject-declarations/func init()

The existing tracer and profiler aspects use inject-declarations to add a func init() { pkg.Start() }, which runs before main(). For the crashtracker, prepend-statements is correct instead, because:

  • SetCrashOutput should be called inside main(), after all package init()s complete. The early-hijack for the monitor child is handled by the crashtracker package's own init() (added in PR1), not the injected call.
  • The injected crashtracker.Start() in main() is what sets up the crash pipe in the application process. It returns immediately if the process is the monitor (already handled) or if DD_CRASHTRACKING_ENABLED=false.

Env-gating:

DD_CRASHTRACKING_ENABLED (default: true) is checked inside Start(). The injected statement is always a single unconditional call; no inline switch needed.

Registration:

The generator at internal/orchestrion/generator/main.go auto-discovers crashtracker/orchestrion.yml (any non-internal directory containing orchestrion.yml). The regenerated orchestrion/all/orchestrion.tool.go now imports the crashtracker package.

Test plan

  • crashtracker/orchestrion.yml validates against the orchestrion schema
  • orchestrion/all/orchestrion.tool.go regenerated and contains _ "github.com/DataDog/dd-trace-go/v2/crashtracker"
  • go build ./crashtracker/... and go build ./orchestrion/... clean
  • Integration test harness (internal/orchestrion/_integration/crashtracker/) added
  • generated_test.go regenerated
  • Full orchestrion CI run (.github/workflows/orchestrion.yml) — will run on PR open

@datadog-datadog-us1-prod

datadog-datadog-us1-prod Bot commented Jul 13, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 62.80% (+0.03%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 2b2f86f | Docs | Datadog PR Page | Give us feedback!

@kakkoyun kakkoyun force-pushed the feat/crashtracker-orchestrion branch 4 times, most recently from 74833fb to e5a4e5e Compare July 13, 2026 15:22
@pr-commenter

pr-commenter Bot commented Jul 13, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-07-14 15:09:43

Comparing candidate commit 2b2f86f in PR branch feat/crashtracker-orchestrion with baseline commit 6a44e0c in branch feat/crashtracker-api.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 325 metrics, 1 unstable metrics, 1 flaky benchmarks without significant changes.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

Known flaky benchmarks

These benchmarks are marked as flaky and will not trigger a failure. Modify FLAKY_BENCHMARKS_REGEX to control which benchmarks are marked as flaky.

Known flaky benchmarks without significant changes:

  • scenario:BenchmarkOTLPTraceWriterFlush

@kakkoyun kakkoyun force-pushed the feat/crashtracker-orchestrion branch 3 times, most recently from e334448 to 4ecfe9e Compare July 14, 2026 11:40
@kakkoyun

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27f8093fd7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

join-point:
all-of:
- package-name: main
- test-main: false

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Instrument the e2e test entrypoint

With test-main: false, this aspect never touches the synthetic go test main. The new e2e fixture re-execs the test binary and its TestMain in package crashtracker panics without calling Start, but that function also does not satisfy package-name: main/function main; the only callable main in that subprocess is therefore excluded here. As a result TestCase.Run waits 15s for a report that can never be produced under orchestrion go test, so CI will fail rather than validating the aspect.

Useful? React with 👍 / 👎.

imports:
crashtracker: github.com/DataDog/dd-trace-go/v2/crashtracker
template: |-
crashtracker.Start()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve manual crashtracker configuration

Because crashtracker.Start is guarded by a package-level sync.Once, this injected no-argument call always wins over any explicit crashtracker.Start(...) later in main. In apps that already configure crash tracking programmatically, those later options become no-ops; for example WithEnabled(false) can no longer disable the monitor and WithAgentURL/WithAPIKey are ignored, so enabling Orchestrion changes an existing crashtracker setup to the defaults.

Useful? React with 👍 / 👎.

imports:
crashtracker: github.com/DataDog/dd-trace-go/v2/crashtracker
template: |-
crashtracker.Start()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Report crashtracker startup failures

This discards the error returned by Start, so in environments where the monitor cannot be spawned or debug.SetCrashOutput fails, Orchestrion-built applications continue with crash reporting silently disabled. Since manual usage in crashtracker/doc.go explicitly checks and logs this error, the injected path should surface the failure as well instead of making crashtracker outages invisible.

Useful? React with 👍 / 👎.

@kakkoyun kakkoyun requested a review from hannahkm July 14, 2026 12:14
kakkoyun added 3 commits July 14, 2026 16:41
Add an orchestrion aspect that prepends crashtracker.Start() as the first
statement of every program's main() function at compile time, enabling
transparent crash report capture without any source changes.

The aspect uses prepend-statements (not inject-declarations/func init())
because SetCrashOutput must run inside main(), after all package init()s
have completed but before any user code executes. This matches the RFC's
requirement for the monitor process to be established before any goroutine
can crash.

Env-gating (DD_CRASHTRACKING_ENABLED, default true) is handled inside
Start() itself, so the injected statement is a single unconditional call.

Regenerates orchestrion/all/orchestrion.tool.go to include the crashtracker
package. Adds a harness integration test confirming the aspect wires correctly
under orchestrion's compile-time transformation.
Replaces the placeholder TestCase with a full crash-reporting chain test:

1. TestCase.Setup() starts a mock Error Tracking intake.
2. TestCase.Run() spawns a subprocess (this same orchestrion-built binary)
   with _CRASHTRACKER_E2E_ORCH=panic.
3. The subprocess panics WITHOUT calling crashtracker.Start() explicitly.
4. The orchestrion-injected Start() (fired before TestMain) spawns the
   monitor grandchild which uploads to the mock intake.
5. The test asserts ddsource, error.type='panic', error.is_crash=true,
   and the correct message.

If orchestrion fails to inject Start(), no monitor is spawned and the
test times out — proving the orchestrion.yml aspect is load-bearing.
…cess

The orchestrion.yml join-point uses test-main:false, which excludes test
binaries' main() from injection. The subprocess re-execs this integration
test binary, so orchestrion never injected Start() — causing the monitor
to never spawn and the test to time out.

Fix: call crashtracker.Start() explicitly in the subprocess role.
The test still validates the full crash pipeline in the orchestrion test
environment. Injection into non-test binaries is covered by the
toolexec/driver/goflags build modes against real application code.
@kakkoyun kakkoyun force-pushed the feat/crashtracker-orchestrion branch from 925e937 to 2b2f86f Compare July 14, 2026 14:41
imports:
crashtracker: github.com/DataDog/dd-trace-go/v2/crashtracker
template: |-
crashtracker.Start()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know how this behaves together with ddtrace/tracer/orchestrion.yml? AFAICT the tracer should start first before crashtracking starts -- how do we enforce that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants