[APMSVLS-501] refactor(bottlecap): preparatory work for "test-mode" binary

[lucaspimentel]

Overview

Preparatory refactors for the upcoming bottlecap-testmode binary (APMSVLS-501), which reuses TraceAgent / handle_traces but has no Lambda lifecycle to drive. Two logical changes:

Update: the bottlecap::startup extraction that was originally part of this PR has moved to #1216, where the bottlecap-test-mode binary that consumes it lands. build_trace_agent had no caller here (only start_trace_agent, in the same file), so the split and the public module were unjustifiable in this diff. main.rs and lib.rs are now untouched by this PR.

1. InvocationProcessorHandle::noop()

Constructor backed by a background task that acknowledges every ProcessorCommand with a sensible default so callers never block on their response oneshots:

• Request-response commands (GetReparentingInfo, UpdateReparenting, SetColdStartSpanTraceId, PlatformRuntimeDone, PlatformReport) reply with empty/default values.
• Fire-and-forget commands are dropped silently.

The match is exhaustive: adding a new ProcessorCommand variant will cause a compile error here, forcing test-mode behavior to be decided explicitly. A response-carrying variant placed in the fire-and-forget arm would silently drop its sender, causing the caller to receive ProcessorError::ChannelReceive instead of the intended default.

The noop channel capacity matches the real InvocationProcessorService to avoid backpressure surprises.

2. RouterExtension seam on TraceAgent

Adds a generic extension point that lets a caller merge additional axum routes into the trace-agent's HTTP router without TraceAgent knowing what those routes do. When no caller attaches an extension (the Lambda binary case), the HTTP surface on port 8126 is unchanged.

• New pub trait RouterExtension: Send + Sync { fn extend(&self, router: Router) -> Result<Router, Box<dyn Error>>; } defined in traces::trace_agent. Returning Err aborts agent startup and surfaces the error in the existing log path, preventing silent failures from a panicking or misconfigured extension.
• TraceAgent holds router_extension: Option<Arc<dyn RouterExtension>> with a consuming builder method with_router_extension(self, ext: Arc<dyn RouterExtension>) -> Self.
• make_router calls extension.extend(router)? when the field is set, before applying the outer fallback and body-limit layers.
• A #[cfg(test)]-only SpyExtension validates the seam end-to-end: the trait shape compiles, state is carried via Arc, and merged routes are reachable through the composed Router returned by make_router.

Any concrete flush/drain/diagnostic endpoint lives behind an impl of this trait in the consumer crate, not in trace_agent.rs.

Testing

• cargo check --bin bottlecap
• cargo check --lib
• cargo test --lib
• cargo clippy --workspace --all-targets --features default -- -D warnings
• cargo fmt --all -- --check
• cargo nextest run --workspace (532/532 passed)
• New unit tests on InvocationProcessorHandle::noop(): noop_request_response_methods_return_defaults, noop_fire_and_forget_commands_do_not_panic, noop_platform_runtime_done_and_report_respond_without_blocking, noop_request_response_variants_complete_within_timeout
• New unit tests on the RouterExtension seam: with_router_extension_adds_reachable_route_to_make_router, make_router_returns_404_for_extension_route_when_none_attached

"Why did the noop handler go to therapy? Because it had to acknowledge every command without ever really doing anything." — Claude 🤖

[Copilot]

Pull request overview

Preparatory refactors to enable an upcoming bottlecap-testmode binary to reuse the trace-processing pipeline (TraceAgent / handle_traces) without relying on the Lambda lifecycle, and to optionally expose a deterministic flush hook for test harnesses.

Changes:

• Added InvocationProcessorHandle::noop() backed by a background task that acknowledges all ProcessorCommand variants with sensible defaults (plus unit tests).
• Promoted start_trace_agent wiring from the Lambda binary into a reusable library module (bottlecap::traces::startup), including a build_trace_agent variant that returns an unspawned TraceAgent.
• Added an opt-in POST /flush route on the trace-agent listener when a FlushingService is attached via TraceAgent::with_flushing_service.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
bottlecap/src/traces/trace_agent.rs	Adds optional FlushingService plumbing and conditionally registers POST /flush.
bottlecap/src/traces/startup.rs	Introduces shared library helpers to build/spawn the trace-agent pipeline.
bottlecap/src/traces/mod.rs	Exposes the new startup module and re-exports pipeline helpers/types.
bottlecap/src/lifecycle/invocation/processor_service.rs	Adds InvocationProcessorHandle::noop() plus tests to ensure defaults/non-blocking behavior.
bottlecap/src/bin/bottlecap/main.rs	Switches Lambda binary to use the new library start_trace_agent helper and removes duplicated wiring.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 723059ecf6

ℹ️ 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".

[duncanista]

this is binary specific code, not traces component related code, you could create a file/module in the bottlecap binary folder instead of here, those methods are supposed to be binary specific, this breaks that pattern

[lucaspimentel]

The reason I moved it here in the library and not under src/bin/bottlecap/ is that this PR is preparation for adding a second binary (a "test mode" bin under src/bin/bottlecap-test/).

Happy to discuss if you have a different sharing pattern in mind!

[lucaspimentel]

You were right, and I've moved it out of this PR.

Looking again at what this PR actually contains: build_trace_agent had no caller here. The only thing calling it was start_trace_agent, in the same file, and the two references from trace_agent.rs were rustdoc links. So the build/start split and the public bottlecap::startup module had no justification visible in this diff, which is exactly what you were reacting to.

start_trace_agent is back in src/bin/bottlecap/main.rs, unchanged from main. main.rs and lib.rs are now untouched by this PR, which is down to 3 files (+474/-6) and just the two pieces that carry their own tests: InvocationProcessorHandle::noop() and the RouterExtension seam.

The extraction moves to #1216, which adds the bottlecap-test-mode binary that imports it. The reason it goes in the library rather than src/bin/bottlecap/ is that two [[bin]] targets can't share a module any other way, but that argument only holds once the second target exists, so it belongs in that PR alongside its consumer.

On "not traces component related" — agreed, and that's also why it sits at the crate root rather than under traces/: it wires trace + stats + proxy + lifecycle + tags + appsec + flushing, so it's cross-cutting orchestration rather than a trace-domain API.

🤖

[duncanista]

Review

Appreciate the thorough PR description and the preparatory structure here. The noop() constructor and build_/start_trace_agent split are solid ideas. However, there are several Rust style and structural issues worth addressing before this merges, especially since this is establishing patterns that the test-mode binary will build on.

---

1. TraceAgentPipeline — 8-element tuple as public API

pub type TraceAgentPipeline = (
    Sender<SendDataBuilderInfo>,
    Arc<trace_flusher::TraceFlusher>,
    Arc<trace_processor::ServerlessTraceProcessor>,
    Arc<stats_flusher::StatsFlusher>,
    Arc<ProxyFlusher>,
    CancellationToken,
    StatsConcentratorHandle,
    TraceAggregatorHandle,
);

In Rust, tuples beyond 3-4 elements are a code smell. At every call site, someone destructures by position — if the order changes, or two fields happen to share a type, it silently breaks. Now that this is being promoted from a private function's return type to a pub type alias in the library, it should be a struct with named fields:

pub struct TraceAgentPipeline {
    pub trace_channel: Sender<SendDataBuilderInfo>,
    pub trace_flusher: Arc<trace_flusher::TraceFlusher>,
    pub trace_processor: Arc<trace_processor::ServerlessTraceProcessor>,
    // ...
}

Named fields are self-documenting, order-independent, and play nicely with IDE autocompletion. The original code had this problem, but promoting it to library API is the right time to fix it.

---

2. startup.rs doesn't belong in traces/

(Expanding on Jordan's inline comment.)

build_trace_agent imports from config, lifecycle, tags, traces, appsec, and flushing — it touches nearly every module in the crate. It's application-level wiring/orchestration, not a traces component. Placing it in traces::startup misrepresents its role and breaks the pattern where traces/ contains trace-domain code.

The existing pattern is that each binary owns its startup wiring in bin/<name>/main.rs. If sharing between the Lambda and test-mode binaries is truly needed, consider:

• A top-level bottlecap::startup module (not nested under a domain module)
• Or a shared helper in bin/ if both binaries live in the same crate

Re-exporting via pub use startup::{...} from traces::mod.rs further blurs the boundary — callers see bottlecap::traces::start_trace_agent and assume it's trace-domain API.

---

3. flush handler — over-engineered timeout logic

The current code (after accepting Copilot's suggestion):

Err(_) => {
    task.abort();
    match task.await {
        Err(join_err) if join_err.is_cancelled() => StatusCode::GATEWAY_TIMEOUT,
        Err(_) => StatusCode::INTERNAL_SERVER_ERROR,
        Ok(()) => StatusCode::GATEWAY_TIMEOUT,
    }
}

After task.abort(), the await will always resolve to Err(JoinError::Cancelled) (the Ok(()) arm is only reachable in a vanishingly small race window, and even then the right status is still 504). The inner match has 3 arms that all effectively return the same thing. Simplify to:

Err(_) => {
    task.abort();
    StatusCode::GATEWAY_TIMEOUT
}

We don't need to await after abort — we already know the status code. Simpler code is also more correct code because there are fewer branches to reason about.

---

4. Noop tests — reach through internals instead of using the public API

The third test (noop_platform_runtime_done_and_report_respond_without_blocking) is 50+ lines that manually construct ProcessorCommand::PlatformRuntimeDone and ProcessorCommand::PlatformReport, sending them through handle.sender directly. But InvocationProcessorHandle already has public methods for both:

• handle.on_platform_runtime_done(...)
• handle.on_platform_report(...)

The test should use those. Testing through the public API is more resilient to internal refactors (e.g., if the channel type changes) and is more readable — the reader immediately understands what behavior is being tested rather than having to mentally map command construction to handler behavior. It also avoids the 15+ lines of field construction for each command variant.

---

5. Doc comments leak consumer knowledge into library functions

build_trace_agent's doc references "bottlecap-testmode binary" and "POST /flush route" repeatedly. Library functions should document what they do and why a caller would use them, not who specifically calls them. Consumer-specific guidance belongs in the consumer's code. For example:

"Returns an unspawned TraceAgent so the caller can configure it (e.g., attach optional routes) before spawning."

is better than:

"notably the bottlecap-testmode binary, which uses it to back a POST /flush route"

---

6. flushing_service: Option<Arc<FlushingService>> on TraceAgent

Adding a test-only field to a production struct is pragmatic for one field, but worth flagging as a pattern to not repeat. Every TraceAgent in production carries a None that exists solely for test-mode. If more test-mode hooks are needed later, consider a trait or a router-extension pattern instead of accumulating Option fields.

---

Summary

Area	Suggestion
TraceAgentPipeline	Replace 8-tuple with a named struct
startup.rs location	Move out of traces/, it's orchestration not trace-domain code
flush timeout	Simplify the post-abort match to just return 504
Noop tests	Use handle.on_platform_runtime_done() / on_platform_report() instead of raw channel sends
Doc comments	Remove consumer-specific references from library function docs
flushing_service field	Fine for now, but flag as not-to-repeat pattern

Happy to discuss any of these — the overall direction is good, these are about getting the Rust patterns right before the test-mode binary builds on top.

[lucaspimentel]

edited 2026-08-26

@duncanista, all six items addressed:

1. TraceAgentPipeline tuple → struct with named pub fields. Destructure by name at the call site, so reordering or same-typed-field swaps can't break silently.

   • Moved to [APMSVLS-501] feat(bottlecap): add bottlecap-test-mode binary #1216

2. startup.rs moved out of traces/ to a new top-level bottlecap::startup. The Lambda binary calls bottlecap::startup::start_trace_agent now, no longer reaching into a domain module; the re-export from traces::mod.rs is gone.

   • Moved to [APMSVLS-501] feat(bottlecap): add bottlecap-test-mode binary #1216

3. POST /flush timeout arm collapsed to task.abort(); 504. The redundant await-after-abort and the unreachable 500 branch are gone; a panic still surfaces through the outer Ok(Err(_)) arm as 500.

   • Superseded by item 6 in this same PR: swapping the flushing_service field for the RouterExtension seam removed the hardcoded /flush handler, and the timeout/abort/504 logic went with it. trace_agent.rs no longer defines a /flush route; the route now lives in [APMSVLS-501] feat(bottlecap): add bottlecap-test-mode binary #1216's FlushRouterExtension.

4. Noop test uses handle.on_platform_runtime_done / on_platform_report instead of constructing ProcessorCommand variants by hand. About 15 lines of per-variant field plumbing removed.

5. Consumer-specific references stripped from library doc comments. No more bottlecap-testmode or POST /flush route naming in with_router_extension, InvocationProcessorHandle::noop, etc. The library docs describe behavior, not today's caller. (The same cleanup on build_trace_agent moved to [APMSVLS-501] feat(bottlecap): add bottlecap-test-mode binary #1216 along with startup.rs.)

6. flushing_service field replaced with a RouterExtension trait. Swapped the Option<Arc<FlushingService>> field for a single generic slot Option<Arc<dyn RouterExtension>> with pub trait RouterExtension { fn extend(&self, router: Router) -> Result<Router, Box<dyn Error>>; }. Returning Err aborts agent startup rather than failing silently. TraceAgent no longer imports or references FlushingService; the flush handler and FLUSH_ENDPOINT_PATH const are gone. Future test-mode hooks become new trait impls (or default-implemented methods added to the trait), not new Option fields. Three cfg(test) tests cover the seam: a spy extension attached via with_router_extension is reachable through make_router; an extension returning Err propagates out of make_router; and the None path falls through to 404.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[datadog-prod-us1-5]

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: b005d31 | Docs | View more details | Give us feedback!}