[APMSVLS-501] feat(bottlecap): add bottlecap-test-mode binary - #1216
[APMSVLS-501] feat(bottlecap): add bottlecap-test-mode binary#1216lucaspimentel wants to merge 4 commits into
bottlecap-test-mode binary#1216Conversation
149d9d4 to
f8da804
Compare
5f17230 to
14cfb71
Compare
|
lucaspimentel
changed the title
bottlecap-test-mode binary
b7bfcb6 to
abb35a3
Compare
4efbc0d to
0145da0
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 0145da0538
ℹ️ 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".
| let mut task = tokio::task::spawn(async move { | ||
| fs.flush_blocking_final().await; | ||
| }); |
There was a problem hiding this comment.
When a client calls /flush immediately after an accepted /v0.4, /v0.5, or /v0.6 request, the payload may still be in TraceAgent's intermediate MPSC channel: the request handlers await enqueueing, but the separate receiver tasks have not necessarily inserted it into the trace or stats aggregator. This flush can therefore issue the aggregator drain first, return 204, and leave the just-accepted payload buffered until another flush, making the parity harness nondeterministic; establish a barrier with both receiver tasks before flushing.
AGENTS.md reference: AGENTS.md:L40-L56
Useful? React with 👍 / 👎.
| stats_aggregator.clone(), | ||
| Arc::clone(config), | ||
| trace_http_client.clone(), | ||
| libdd_trace_utils::config_utils::trace_stats_url(&config.site), |
There was a problem hiding this comment.
When the parity harness points the advertised DD_APM_DD_URL at fake-intake and submits client-computed data to /v0.6/stats, this constructs the stats destination solely from DD_SITE, so traces reach fake-intake while stats are sent toward a Datadog stats host with the stub API key. The new binary consequently cannot exercise its advertised stats accept/aggregate/flush path against the local intake; test mode needs a stats URL override or must derive the fake-intake stats endpoint from its custom APM destination.
Useful? React with 👍 / 👎.
| match tokio::time::timeout(FLUSH_REQUEST_TIMEOUT, &mut task).await { | ||
| Ok(Ok(())) => StatusCode::NO_CONTENT, |
There was a problem hiding this comment.
If fake-intake is unavailable or returns a non-success status, TraceFlusher, StatsFlusher, and the proxy/log flushers return retry payloads rather than panicking, while FlushingService::flush_blocking_final() discards every result and still returns (). The join therefore succeeds and this branch returns 204 even though data was drained and not delivered, giving the harness a false successful-drain signal; propagate the flusher results and map any exhausted send failure to a non-2xx response.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| shutdown_token.cancel(); | ||
| flushing_service.flush_blocking_final().await; |
There was a problem hiding this comment.
When SIGINT arrives while a trace or stats request is in flight, cancellation only signals axum's graceful-shutdown future; the listener's discarded JoinHandle is not awaited before flush_blocking_final() starts. The flush can drain the aggregators before the request handler finishes processing and before the intermediate channel receivers forward its payload, after which main exits and drops that data; retain and await the listener task before performing the final flush.
AGENTS.md reference: AGENTS.md:L40-L56
Useful? React with 👍 / 👎.
| tokio::spawn(async move { | ||
| if let Err(e) = trace_agent.start().await { | ||
| error!("Error starting trace agent: {e:?}"); | ||
| } |
There was a problem hiding this comment.
If port 8126 is already occupied or axum otherwise fails during startup, this detached task only logs the error and terminates while main continues waiting indefinitely for Ctrl-C. A harness therefore sees a live test-mode process with no listener—or may accidentally connect to the process already using the port—instead of a startup failure; propagate the listener result to main or coordinate startup through a channel.
Useful? React with 👍 / 👎.
abb35a3 to
211d68c
Compare
Moves start_trace_agent out of the Lambda binary into bottlecap::startup so both [[bin]] targets can share it, and splits it into: - build_trace_agent, which returns an unspawned TraceAgent plus a TraceAgentPipeline handle struct - start_trace_agent, a thin wrapper that spawns the agent bottlecap-test-mode needs the unspawned agent so it can attach its /flush RouterExtension before spawning; the Lambda binary keeps calling start_trace_agent and its call site is unchanged. Placed at the crate root rather than under traces/ because it wires trace, stats, proxy, lifecycle, tags, appsec, and flushing together.
A second [[bin]] target that runs the APM trace-processing surface as a long-lived HTTP server with no Lambda lifecycle. Listens on 127.0.0.1:8126 and exposes the standard tracer endpoints (/v0.4/traces, /v0.5/traces, /v0.6/stats, /info) plus POST /flush for deterministic harness-driven flushing. Configured by the same DD_* env vars the Lambda binary reads. Optional periodic flushing via DD_SERVERLESS_FLUSH_STRATEGY (decoupled from managed-instance mode). Gated behind the `test-mode` cargo feature (required-features), so it is not built in default or fips builds. Build with `cargo build --bin bottlecap-test-mode --features test-mode`. Intended for the cross-agent parity harness (APMSVLS-496) and for local dev workflows that need a tracer endpoint without standing up a Lambda. APMSVLS-501 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
flush_blocking_final expects on the metrics aggregator handle, so a dead aggregator task panicked the connection task and the harness saw a dropped connection instead of a status it could act on. The five flushers also bound only their individual HTTP calls, so stacked retries could leave a request outstanding far longer than a harness should wait. Runs the flush in a spawned task and caps it at 30s: 204 on success, 500 if the task panics, 504 after aborting a timed-out flush. Restores the hardening that previously lived in the trace agent's hardcoded /flush handler, now on the consumer side where the route lives.
The bottlecap-test-mode binary is gated behind required-features, so no existing CI job compiles it and a change to library code could break it without any job failing. Add a clippy pass with the test-mode feature enabled to both the GitHub Actions and GitLab pipelines.
0145da0 to
b0f9bcc
Compare
1 participant
Part of a PR stack:
bottlecap-test-modebinary #1216 👈🏽 this PROverview
Adds a second
[[bin]]target,bottlecap-test-mode, that runs the APM trace-processing surface as a long-lived HTTP server with no AWS Lambda Extension lifecycle. ReusesTraceAgent,FlushingService, and the trace/stats/proxy flushers built bybottlecap::startup::build_trace_agent, which this PR extracts from the Lambda binary (see below); the only Lambda-binary code it duplicates isinit_ustrandenable_logging_subsystem(~20 lines, called out in the design doc).Endpoints on
127.0.0.1:8126:/v0.4/traces,/v0.5/traces,/v0.6/stats,/info— unchanged from the Lambda binary'sTraceAgentrouter.POST /flush— new, registered by aFlushRouterExtensionimpl attached viaTraceAgent::with_router_extension(...)(the seam from [APMSVLS-501] refactor(bottlecap): preparatory work for a new "test-mode" binary #1344). CallsFlushingService::flush_blocking_final()in a spawned task bounded at 30s:204 No Contenton success,500if the flush panics,504after aborting a timed-out flush.flush_blocking_finalexpect()s on the metrics aggregator handle, so without the spawn a dead aggregator would panic the connection task and the harness would see a dropped connection rather than a status it can act on; the per-flusher HTTP timeouts also bound only individual calls, so stacked retries could otherwise outlive any sensible harness wait.Configuration: same
DD_*env vars as the Lambda binary. Notable inputs:DD_APM_DD_URL— redirects trace intake (the parity harness points this at the fake-intake from [APMSVLS-497][APMSVLS-498] test: add fake-intake for APM payload-level tests #1194).DD_SERVERLESS_FLUSH_STRATEGY— opt-in periodic flush ticker, decoupled from managed-instance mode.DD_TESTMODE_FUNCTION_ARN— overrides the stub ARN used for tag generation (defaults toarn:aws:lambda:us-east-1:000000000000:function:testmode).API key is hardcoded to
"stub-key"(no secrets resolver path); the parity harness fake-intake ignores auth.bottlecap::startupextractionThis PR also promotes
start_trace_agentout ofsrc/bin/bottlecap/main.rsinto a new top-level library module,bottlecap::startup, and splits it into:build_trace_agent— returns an unspawnedTraceAgentplus aTraceAgentPipelinestruct with namedpubfields (flushers, trace-channel sender, shutdown token, aggregator/concentrator handles).start_trace_agent— thin wrapper that spawns the agent. The Lambda binary's call site is unchanged.bottlecap-test-modeneeds the unspawned agent so it can attach its/flushRouterExtensionbefore spawning, which is why the split exists. Placed at the crate root rather than undertraces/because it wires trace, stats, proxy, lifecycle, tags, appsec, and flushing together: cross-cutting orchestration, not a trace-domain API.This extraction was originally part of #1344 and moved here after review feedback: with no second binary in that PR,
build_trace_agenthad no caller and the public module looked unmotivated. Landing it next to its first consumer makes the shared-library placement self-evident. Behavior is unchanged for the Lambda binary;trace_agent.rschanges are two retargeted rustdoc links.Feature gating
Gated behind the
test-modecargo feature viarequired-features = ["test-mode"], the same feature that gatesInvocationProcessorHandle::noop()(added in #1344). The binary is therefore not built indefaultorfipsbuilds, including bycargo build --workspace. Build/run with:No CI job currently builds or lints with
--features test-mode, so this binary is invisible to CI as things stand. Tracked in Follow-ups below.Shutdown ordering
signal::ctrl_c()cancels the shutdown token first (drives axum'sgraceful_shutdownso any in-flight/v0.4/tracesrequest drains through the trace aggregator), then runsflush_blocking_final(). The periodic flush task selects on the same token so it doesn't leak when the listener stops.Why a second binary
The overlap between Lambda-mode and test-mode is small (~20 lines:
init_ustr, logging, config load), and the rest is intentionally different (no telemetry listener, no LWA, no logs agent, no proxy, no DogStatsD UDP, no event-bus-driven lifecycle). A second[[bin]]makes the test-mode surface explicit and structural, enforced by the compiler instead of by a runtime branch. Rejected alternatives (env-var branch, auto-detect, CLI flag, single binary with mode gating) are in the design doc.Design doc:
lucas-pimentel/docs/bottlecap-test-mode.md(local; happy to land it in-repo if reviewers prefer).Testing
cargo check --bin bottlecap-test-mode --features test-modecargo clippy --workspace --all-targets --features default -- -D warnings(existing surface unchanged)cargo clippy --workspace --all-targets --features default,test-mode -- -D warningscargo fmt --all -- --checkcargo test -p bottlecap --lib— 548 passed (unchanged from [APMSVLS-501] refactor(bottlecap): preparatory work for a new "test-mode" binary #1344; the extraction is a pure move):8200:POST /v0.4/traces(real msgpack span) → 200, buffered, periodic flush fired at 2s, intake receivedPOST /api/v0.2/traces(485 bytes,DD-API-KEY: stub-key).POST /flush→ 204. (Predates the timeout/panic guard in4efbc0dd; the 500 and 504 paths were verified separately against an extracted copy of the handler, not against the running binary.)POST /v0.5/traces(malformed) → 500 (correct rejection from the existing v0.5 deserializer).GET /info→ 200 with the standard endpoints list.TRACE_AGENT | Shutdown signal received, shutting down→Aggregator service stopped→ clean exit.No new unit tests in this PR. The seam (
RouterExtension) and the no-op handle (InvocationProcessorHandle::noop()) are both covered by tests added in #1344; end-to-end coverage for test-mode lands as part of the parity harness (#1194 and the futureapm-agent-parity-rsrepo).Follow-ups
Known gaps, called out so they are not mistaken for oversights. Neither blocks the binary from working; happy to fold either into this PR if reviewers prefer.
--features test-mode. Existing jobs run--features defaultand--no-default-features --features fips, andcargo build --allskipsrequired-featurestargets, so nothing in CI compiles this binary. A clippy/build step passing--features test-modewould keep it green; without it, a change to library code can break this target without any job failing. Addressed: acargo clippy --workspace --features default,test-modestep was added to both the GitHub Actions and GitLab pipelines.POST /flushguard in4efbc0ddhas no committed test. The 204/500/504 paths were verified manually, but the test-mode binary has no#[cfg(test)]module. Testing it in place needs the handler factored out of the closure, orFlushingServicemade injectable, so it is deliberately deferred rather than bolted on.🤖 Generated with Claude Code