[APMSVLS-501] feat(bottlecap): add bottlecap-test-mode binary - #1216
Open
lucaspimentel wants to merge 26 commits into
Open
lucaspimentel wants to merge 26 commits into
lucaspimentel wants to merge 26 commits into
Conversation
lucaspimentel
force-pushed
the
lpimentel/bottlecap-test-mode
branch
from
August 26, 2026 16:58
149d9d4 to
f8da804
Compare
lucaspimentel
force-pushed
the
lpimentel/bottlecap-testmode-binary
branch
from
August 26, 2026 20:15
5f17230 to
14cfb71
Compare
6 tasks
|
🔗 Commit SHA: b08fde8 | Docs | View more details | Give us feedback! |
6 tasks
lucaspimentel
changed the title
bottlecap-test-mode binary
lucaspimentel
force-pushed
the
lpimentel/bottlecap-test-mode
branch
from
September 2, 2026 20:29
b7bfcb6 to
abb35a3
Compare
lucaspimentel
force-pushed
the
lpimentel/bottlecap-testmode-binary
branch
from
September 2, 2026 20:56
4efbc0d to
0145da0
Compare
Member
Author
|
@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".
lucaspimentel
force-pushed
the
lpimentel/bottlecap-test-mode
branch
from
September 3, 2026 21:20
abb35a3 to
211d68c
Compare
lucaspimentel
force-pushed
the
lpimentel/bottlecap-testmode-binary
branch
from
September 3, 2026 21:22
0145da0 to
b0f9bcc
Compare
lucaspimentel
force-pushed
the
lpimentel/bottlecap-test-mode
branch
2 times, most recently
from
September 9, 2026 15:22
901ea64 to
e852fe4
Compare
lucaspimentel
force-pushed
the
lpimentel/bottlecap-testmode-binary
branch
from
September 9, 2026 16:31
b0f9bcc to
f7dbb9d
Compare
lucaspimentel
force-pushed
the
lpimentel/bottlecap-test-mode
branch
from
September 10, 2026 15:30
e49b50d to
1976b21
Compare
lucaspimentel
force-pushed
the
lpimentel/bottlecap-testmode-binary
branch
from
September 10, 2026 18:49
f7dbb9d to
c559152
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent flushes can report success or exit while an earlier destructive flush remains incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a feature-gated, long-running APM test-mode server while sharing the production trace pipeline.
Changes:
- Adds trace ingestion, manual/periodic flushing, and graceful shutdown.
- Extracts shared trace-agent startup and adds ingestion barriers.
- Adds flush failure reporting and CI coverage for
test-mode.
File summaries
| File | Description |
|---|---|
.github/workflows/rs_ci.yml |
Checks the test-mode feature. |
.gitlab/templates/pipeline.yaml.tpl |
Checks the test-mode feature. |
bottlecap/Cargo.toml |
Registers the binary and Tokio signal support. |
bottlecap/src/bin/bottlecap-test-mode/main.rs |
Implements the test-mode server. |
bottlecap/src/bin/bottlecap/main.rs |
Uses shared startup assembly. |
bottlecap/src/flushing/service.rs |
Reports undelivered blocking flushes. |
bottlecap/src/lib.rs |
Exposes the startup module. |
bottlecap/src/startup.rs |
Builds and starts the shared trace pipeline. |
bottlecap/src/traces/trace_agent.rs |
Adds trace/stats ingestion barriers. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical flush and ingest-barrier issues, lifecycle defects, and missing feature-enabled CI tests remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
bottlecap/src/traces/trace_agent.rs:245
- With
biased, this branch is always preferred whenevertrace_rxis ready. Under sustained trace traffic the payload channel can remain ready indefinitely, starving the barrier branch;/flushcan then wait until its 30-second timeout despite a healthy forwarder. Use an ordering/drain protocol that preserves all payloads before the barrier while guaranteeing barrier progress.
bottlecap/src/traces/trace_agent.rs:275 - With
biased, this branch is always preferred wheneverstats_rxis ready. Under sustained stats traffic the payload channel can remain ready indefinitely, starving the barrier branch;/flushcan then wait until its 30-second timeout despite a healthy forwarder. Use an ordering/drain protocol that preserves all payloads before the barrier while guaranteeing barrier progress.
.github/workflows/rs_ci.yml:86
- This CI change only compiles the new required-features binary with Clippy. The test job at
rs_ci.yml:118-147still runscargo nextest run --workspacewithout--features test-mode, so the new binary's#[cfg(test)]tests—including the/flush204/502/500/504 cases—are skipped in CI. Add a feature-enabled test invocation or matrix so these tests actually run.
# The test-mode feature gates the bottlecap-test-mode binary via
# required-features, so no other job compiles it.
bottlecap/src/bin/bottlecap-test-mode/main.rs:273
FlushControl::get_flush_interval()returns a Tokio interval whose first tick is immediate. Unlike the production flush loops (main.rs:493-495andmain.rs:675), this loop does not discard that tick, soperiodically,<n>triggers an extra flush at startup instead of waiting for the configured interval. Consume the initial tick before entering the loop.
tokio::spawn(async move {
loop {
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
lucaspimentel
force-pushed
the
lpimentel/bottlecap-testmode-binary
branch
3 times, most recently
from
September 16, 2026 16:33
311e9cc to
92618ef
Compare
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.
Trace and stats requests only await the hand-off into an intermediate channel, so a request can be answered while its payload is still queued ahead of the aggregator. A flush issued right after an accepted request could therefore miss it, leaving the payload buffered until the next flush and making the parity harness nondeterministic. Add an ingest barrier over both forwarder tasks and await it in POST /flush and in the shutdown drain. 🤖
DD_APM_DD_URL only moved the trace intake, so a harness pointing traces at a local fake-intake still sent stats toward Datadog with the stub API key, leaving the binary's advertised /v0.6/stats path unexercised. Derive the stats endpoint from the resolved trace intake in test mode. With DD_APM_DD_URL unset this reproduces the site-derived default, and production routing is unchanged. 🤖
The blocking flush path discarded every flusher result, so payloads that could not be delivered were dropped while POST /flush still answered 204. The harness read that as a successful drain. Report undelivered payloads from the blocking flush, log which domains dropped data, and answer 502 instead of 204 when any did. 🤖
Cancelling the shutdown token only signals graceful shutdown, so the final drain could run while a request handler was still processing and its payload would be dropped when the process exited. Await the listener task, bounded so a lingering connection cannot wedge shutdown. 🤖
A failure to bind port 8126 was only logged, leaving a live process with no listener while main waited on ctrl-c. The harness saw a healthy process, or connected to whatever already held the port. Propagate the listener error and race it against ctrl-c so startup failures end the process with a non-zero exit. 🤖
Each flusher drains its aggregator before awaiting network delivery, so the periodic driver, POST /flush, and the shutdown drain could overlap and let one report success on empty queues while another's send was still in flight. All three now run the barrier-plus-flush sequence under a shared lock, which also makes the final drain wait for an in-flight periodic flush. 🤖
A closed barrier channel or a dropped acknowledgement means the forwarder task is gone and the payloads queued ahead of it were lost, not drained. The barrier now returns an error instead of treating that as success, and POST /flush reports 502 rather than 204. 🤖
The endpoint table listed only the five core paths, but the binary also serves every proxy route the trace agent's router registers. 🤖
The handler now takes the flush work as an injectable operation, so the 204, 502, 500 and 504 branches can be driven directly. Without this a regression in the status the harness reads would pass CI. 🤖
The ingest barrier held only its own acknowledgement channels, while every payload sender for the stats forwarder lived inside the trace agent. On the normal test-mode shutdown path the agent is dropped before the final drain runs, so the stats forwarder had already exited (after draining its queue) and the barrier reported it as dead, logging that accepted payloads were lost on every clean shutdown even though nothing was lost. The barrier now keeps a payload sender for each forwarder, so a live barrier keeps both forwarders running and a barrier error again means a forwarder genuinely died. The stats route is also no longer passed in to the router builder, so it cannot be wired to a channel outside the barrier's coverage. 🤖
Only SIGINT was handled, so a container runtime or harness stopping the binary with SIGTERM killed it outright and the final drain never ran, losing the last accepted payloads. The signal driver is now pulled in by the test-mode feature rather than unconditionally, so the shipped extension no longer carries it. Also corrects the periodic flush comment: the `end` strategy yields the placeholder interval that means "never race a flush", so it is periodic only in name. 🤖
The path the config crate appends to the trace intake URL was declared twice, once for data streams and once for the test-mode binary. Both now use a single definition. 🤖
The test-mode binary now reads DD_APM_RECEIVER_PORT (the same variable the Go trace agent uses) and binds its listener to that port instead of the default 8126. Unset or empty keeps the default; invalid values fail startup rather than silently listening on the wrong port. Production Lambda behavior is unchanged.
Extract the shared select-loop scaffolding behind the trace and stats forwarder tasks into one generic helper. Also wait for both forwarders concurrently in IngestBarrier::wait instead of sequentially, since they are independent tasks with no ordering dependency, roughly halving the barrier's contribution to flush latency. 🤖
Both looked like unnecessary complexity on review, but dropping the handler (e.g. on request cancellation) relies on JoinSet aborting its child task, which a bare JoinHandle would not do, and select! evaluates branch futures eagerly, so calling flush_op() outside the async block would run it even when the cancellation branch wins. 🤖
lucaspimentel
force-pushed
the
lpimentel/bottlecap-testmode-binary
branch
from
September 22, 2026 22:54
14b8be7 to
b08fde8
Compare
lucaspimentel
marked this pull request as ready for review
September 23, 2026 23:09
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.
Part of a PR stack:
bottlecap-test-modebinary #1216 👈🏽 this PROverview
Adds a
bottlecap-test-modebinary: the APM trace-processing surface as a long-lived HTTP server, with no Lambda Extension lifecycle. It powers the APM parity harness (#1194), which drives fixture payloads through three agents and diffs what reaches the fake intake, and enables local tracer debugging without Lambda RIE.Why
Today, bottlecap's trace processing is only reachable through a live Lambda invocation: the trace agent runs inside the extension's lifecycle, so exercising it requires Lambda RIE, and flush timing is whatever the invocation dictates. The parity harness needs a deterministic target it can drive with fixture payloads and diff against a fake intake, and local tracer debugging should not require standing up RIE. This binary exposes the same pipeline as a plain HTTP server with explicit flush control via
POST /flush.Design
[[bin]]instead of a runtime branch: it shares only ~20 lines with the Lambda binary (config, logging); a separate target makes the reduced surface (no telemetry listener, LWA, logs agent, proxy, DogStatsD) compiler-enforced. Gated behind thetest-modefeature, sodefault,fips, andcargo build --workspaceare unaffected.TraceAgent,FlushingService, and the trace/stats flushers, so it exercises the same code paths.start_trace_agentmoved into abottlecap::startuplibrary module, split intobuild_trace_agent(unspawned agent + named handles) and the spawning wrapper, because test-mode must attachPOST /flushbefore spawning. The Lambda call site is unchanged./v0.4/traces,/v0.5/traces,/v0.6/stats,/infocome unchanged fromTraceAgent's router (proxy routes included).POST /flushdrains the ingest barrier and runs a final blocking flush, bounded at 30s:204on full delivery,502on lost payloads,504on timeout,500on panic.DD_*env vars as Lambda.DD_APM_DD_URLredirects the intake,DD_APM_RECEIVER_PORToverrides the listener port (the Go trace agent's variable; invalid values fail startup),DD_SERVERLESS_FLUSH_STRATEGYopts into a periodic flush ticker,DD_TESTMODE_FUNCTION_ARNoverrides the stub ARN. API key is hardcoded to"stub-key".Determinism fixes in the library crate
The harness must observe the right answer through HTTP alone. Four gaps, none of which change production behavior:
traces/trace_agent.rs): a200could return while the payload was still queued, so an immediate/flushmissed it. Forwarders now only acknowledge when their channel is empty, and/flushawaitsTraceAgent::ingest_barrier()first. A dead forwarder reports loss instead of success.startup.rs): otherwise a redirected harness sent stats to Datadog with the stub key, leaving/v0.6/statsunexercised.flushing/service.rs): the blocking flush discarded all flusher results, so/flushanswered204after dropping payloads. It now returns whether any flusher gave up./flush, and shutdown drain shared oneFlushingService, letting one report success on empty queues while the other's send was still in flight. All three run under a shared lock.Shutdown
Exits non-zero if the listener fails to bind instead of idling. SIGINT/SIGTERM cancel the listener (10s grace, then forced abort, since Axum connection tasks can outlive it), await the ingest barrier, and run the final blocking flush.
Testing
cargo clippy --workspace --all-targetsfordefaultanddefault,test-modewith-D warnings;cargo fmt --checkcargo nextest run --locked --offline --workspace --features test-mode --test-threads 1: 702 passedPOST /flushstatus contract, ingest barrier, stats URL override, receiver-port parsing, shutdown/cancellation regressions)/v0.4/tracesflushed to a local fake intake; malformed payload → 500; bound port → clean startup failure; SIGINT → clean drainlogs::flusher::tests::send_bounds_retries_on_transport_erroris flaky under concurrent runs only; passes in isolation and in serial runs.End-to-end coverage lands with the parity harness (#1194 and the future
apm-agent-parity-rsrepo).Follow-ups
--features default,test-mode(GitHub Actions + GitLab) and explicitly runs the test-mode binary tests.POST /flushstatus contract is covered through the router with an injectable flush operation.