feat: inspectable tracer#4512
Merged
Merged
Conversation
This allows to swap the flush handling on the tracer with other implementations that require different flush semantics, like synchronous ones.
…integration tests to use {test,agent}test
darccio
force-pushed
the
dario.castane/rd/inspectable-tracer-v2
branch
from
March 9, 2026 15:03
8212d72 to
3c74aaf
Compare
🎉 All green!🧪 All tests passed 🔄 Datadog auto-retried 1 job - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: e22ba01 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-07-09 18:12:41 Comparing candidate commit 7088e49 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 323 metrics, 1 unstable metrics, 1 flaky benchmarks without significant changes.
|
…es by replacing FreePort+ListenAndServe with a pre-bound FreeListener+Serve and adding a TestCasePreBootstrap interface so os/lfi.go can configure AppSec env vars before the tracer starts
Member
Author
|
@codex review |
…Tags to attributes, Meta and Metrics to agenttest span tags
…n to expected traces
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 078b37add2
ℹ️ 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".
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f6465bb5f
ℹ️ 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".
Codecov Report❌ Patch coverage is Additional details and impacted files
🚀 New features to boost your workflow:
|
Member
Author
|
Sorry, something went horrible while merging main. |
SpanMeta is now a struct (not map[string]string) so len() and range no longer compile. Use maps.Clone(span.meta.Map(true)) for a safe owned copy, Count() for sizing Tags, and iterate the cloned map. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
The three broken tests were written against a planned `testtracer` package (WithMockResponses, WithTracerStartOpts, WaitForLLMObsSpans) that does not exist yet, causing build failures across all packages that import the tracer. Fixes: - Add SpanBatchSizes() to llmobstest.Collector to expose raw HTTP body sizes for each span batch, recorded in handleSpans alongside span collection. - Rewrite TestSpanEventsSizeBasedFlushing (llmobs_test.go): use existing testTracer helper; replace testtracer.Start + tt.WaitForLLMObsSpans with tracer.Flush + coll.SpanCount/SpanBatchSizes assertions. - Rewrite TestLargeDatasetPushChunking (dataset_test.go): inline tracer setup with tracertest.StartAgent + llmobstest.New; convert round-tripper-style mock (func(*Request)*Response) to http.HandlerFunc registered via coll.HandleFunc; fix createMockHandler()(w, r) call signature. - Rewrite TestExperimentLargeDatasetSizeBasedFlushing (experiment_test.go): same pattern using tracertest.Bootstrap; LLMObs span batch sizes come from coll.SpanBatchSizes() instead of intercepted request bodies. - Remove unused imports (bytes, io, sync) from llmobs_test.go and experiment_test.go left over from the removed mock closures. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…size The mock handler always returned `numRecords` entries, but dataset.Push chunks large datasets into smaller batches. Transport validates that `len(resp.Data) == len(insert) + len(update)`, so a fixed-size response caused a mismatch on all but the first chunk, silently aborting the push after 3 spans instead of 12. Parse the request body to count the actual records per chunk and echo back exactly that many response entries. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Size-based flushes fired `l.wg.Go(batchSend)` without incrementing `sendWg`, so `FlushSync` → `sendWg.Wait()` returned immediately even with in-flight size-based sends. On a slow CI runner this was a reliable race: `FlushSync` returned, the test read `SpanCount()`, and the background goroutines hadn't yet delivered their batches to the collector — yielding "expected 12, actual 3". Fix: add `sendWg.Add(1)` / `defer sendWg.Done()` around size-based flush goroutines, matching the pattern used by ticker and flushNow paths. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Add withForceAgentWriter() option that sets OTLPExportMode=false before newTracer selects the traceWriter. Without this, OTEL_TRACES_EXPORTER=otlp in the environment routes all test spans to the remote OTLP endpoint instead of the in-process test agent, causing every span assertion to fail with "collected 0 span(s)". withAgentTransport only overrides the HTTP client; OTLP mode selects a different writer that ignores c.httpClient entirely, so a separate guard is needed. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
sendWg existed only to let FlushSync call sendWg.Wait() from inside the main run loop goroutine without deadlocking on wg.Wait() (the main loop was itself tracked by wg). Fix the root cause: start the main loop as a plain goroutine and close stoppedCh on exit. wg now tracks only async batchSend goroutines, so FlushSync can call wg.Wait() directly without deadlock. Stop() waits for the main loop via <-stoppedCh, then for remaining async sends via wg.Wait(). Extracts sendAsync() to remove the repeated Add/Go/Done boilerplate at each call site, making it impossible to accidentally skip tracking a future async send. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…tusUpdates to tracertest API Main added multi-run and status-update experiment tests using the old testtracer round-tripper API. This branch replaced testtracer with llmobstest.Collector + tracertest.Bootstrap. Resolve conflicts by: - Resolving 3 conflict blocks in createMockHandler (add PATCH route for status updates, keep HandlerFunc style) - Adding testTracerWithHandler helper for tests needing a custom LLMObs handler - Adapting TestExperimentMultiRun: drop tt.Stop(), replace WaitForLLMObsSpans with tracer.Flush()+coll.Spans(), rewrite custom-handler subtests to use testTracerWithHandler - Adapting captureStatusUpdates to return http.HandlerFunc instead of testtracer.MockResponseFunc - Adapting TestExperimentStatusUpdates: replace testtracer.WithMockResponses pattern with testTracerWithHandler+captureStatusUpdates - Adding Collector.Spans() to llmobstest for bulk span iteration in tag checks Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…racertest API Replace old testtracer.WaitForSpans/WaitForLLMObsSpans calls (from main merge) with tracer.Flush() + coll.RequireSpan() in two TestStartSpan and TestDDAttributes subtests that were left using the removed testtracer API. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
4 tasks
9 tasks
Resolves conflicts and the semantic-merge breakage caused by main's new LLMObs features colliding with this branch's inspectable-tracer test-harness migration: - ddtrace/tracer/option.go: keep both the branch's test-helper options (withLLMObsInProcessTransport, withAgentTransport, withForceAgentWriter) and main's WithSpanPool. - ddtrace/tracer/tracer_test.go: keep the inspectable-tracer flow (tracer.Flush + agent.FindSpan) using main's named-field chunk literals. - internal/llmobs/llmobs.go: keep the branch's flushSyncCh-based FlushSync design (wg/stoppedCh/sendAsync), discarding main's sendWg/workerDone variant; preserve main's costTags/toolVersion/projectID additions. - internal/orchestrion/_integration/go.mod: take main's dependency bumps (agent 0.79.0, v2.10.0-dev, etc.); reconciled via go mod tidy. - llmobs/dataset/dataset_test.go: keep the new tracertest/llmobstest harness and update record mocks to main's v2 records endpoint + flat format; port main's pull-with-version-option test to the new harness. - internal/llmobs/llmobs_test.go, llmobs/llmobs_test.go, llmobs/experiment/evaluator_result_test.go: port main's new tests (tool-version, cost-tags, FlushSync, evaluator-result) to the new harness. Dropped the FlushSync does-not-hang-after-stop subtest, which probes main's FlushSync internals not guaranteed by this branch's design. - ddtrace/x/llmobstest/collector.go: add Spans() accessor and SetSpanResponseDelay to support the ported tests.
…ectable tracer Extract the remote-config client build + AppSec start block from Start into startAppSec(), then call it from both Start and startInspectableTracer. This ensures WithAppSecEnabled and friends activate identically in both paths, closing the gap flagged in PR #4512 review. Telemetry, runtime metrics, and storeConfig are intentionally kept out of the shared path: they spawn background goroutines and process-global side effects that break the inspectable tracer's synctest/no-network guarantees.
…writers Replace the fragile type switch (agentTraceWriter | otlpTraceWriter) in the inspectable-tracer flush handler with a flushWaiter optional interface. All async writers (agentTraceWriter, otlpTraceWriter, ciVisibilityTraceWriter) implement wait() via their wg.Wait(); logTraceWriter (synchronous) does not. This closes two gaps from the PR #4512 review: ciVisibilityTraceWriter was missing from the switch (flush could return before async sends completed), and the switch is fragile against future writer additions.
After Stop() exits the worker goroutine, a subsequent FlushSync() would block forever on the unbuffered flushSyncCh send. Add a stopCh-guarded select so FlushSync returns immediately once the instance is stopped. Add does-not-hang-after-stop subtest to TestFlushSync (the remaining subtest from the PR #4512 review that was not yet covered by the existing suite).
…t unit tests Three minor fixes surfaced during code review of the inspectable-tracer PR: - ddtrace/tracer/option.go: replace three stale references to a non-existent "finishConfig" function (the actual function is "newConfig") in the comments for agentTransport, llmobsHTTPClient, and withAgentTransport. A future reader searching for finishConfig would find nothing. - ddtrace/x/agenttest/span.go: document that Span.TraceID holds only the lower 64 bits of the trace ID for 128-bit traces; the upper 64 bits are available in Meta["_dd.p.tid"]. Tests asserting full 128-bit identity should use that tag. - ddtrace/x/agenttest/agenttest_test.go (new): direct unit tests for the agenttest package — SpanMatch conditions and AND-semantics, FailedConditions, FindSpan (no match / first match / multi-condition), RequireSpan (found / not found), CountSpans, and HandleTraces in-process round-trip. The package had no test files; CONTRIBUTING targets ~90% coverage for new code.
…ectable-tracer-v2 # Conflicts: # ddtrace/tracer/writer.go # internal/orchestrion/_integration/go.mod
Contributor
Config Audit |
Two CI failures on the inspectable-tracer branch: - contrib mark3labs/mcp-go and modelcontextprotocol/go-sdk test files still called llmobstest.Collector.Stop()/WaitForLLMObsSpans(), which no longer exist. Use a bare testTracer(t) (cleanup is automatic via tracertest.Bootstrap) and tracer.Flush() + Collector.Spans() instead. - os/cmdi.go set DD_APPSEC_* in Setup, which now runs after the tracer bootstraps, so RASP was never armed (403 expected, 500 got). Move the skip checks and appsec env vars into PreBootstrap, mirroring os/lfi.go.
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
2 participants
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.
What does this PR do?
Adds an inspectable tracer that replaces the other four different ways to inspect spans and mocking the tracer.
Migrates a few example tests that we using any of the different existing approaches in the codebase.
Important
testracer.Startstarts a test tracer but it doesn't register it as global tracer.testtracer.Bootstrapstarts a test tracer, a test agent, and registers the former as global tracer.ddtrace/tracerAPI.Motivation
Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.Unsure? Have a question? Request a review!