feat(request-trace): native S3 sink for request-trace records - #11806
Conversation
WalkthroughAdds an optional S3 request-trace sink with environment-based configuration, feature-gated AWS dependencies, asynchronous JSONL batching, gzip compression, S3 uploads, shutdown handling, and key-formatting tests. ChangesS3 request-trace sink
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
9f67d5e to
94335ab
Compare
|
🎯 Code Coverage (details) 🔗 Commit SHA: fedc2da | Docs | Datadog PR Page | Give us feedback! |
94335ab to
03d2e23
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/llm/src/request_trace/s3_sink.rs`:
- Around line 6-9: Update the module documentation describing S3 object keys to
match the implemented format `{host}-{HHMMSS}-{run_id}-{seq}.jsonl.gz`, while
preserving the existing prefix and date-partition layout.
- Around line 155-168: In the shutdown branch of the request-trace worker, close
the receiver before draining pending records, then consume records with recv()
until it returns None instead of using try_recv(). Preserve the existing
serialization warning and final upload_ready_batch behavior, ensuring emits
racing with shutdown cannot enqueue records after draining begins.
In `@lib/runtime/src/config/environment_names.rs`:
- Around line 549-559: Update the documentation for DYN_REQUEST_TRACE_SINKS to
include s3 in its supported sink values and mapping description, while
preserving the existing documented values and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2b3ab66d-b65a-4749-8383-7aae911a3709
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
lib/llm/Cargo.tomllib/llm/src/request_trace/config.rslib/llm/src/request_trace/mod.rslib/llm/src/request_trace/s3_sink.rslib/llm/src/request_trace/sink.rslib/runtime/src/config/environment_names.rs
03d2e23 to
f5738a0
Compare
|
/ok to test f5738a0 |
f5738a0 to
fe3d847
Compare
|
/ok to test c3a50dc |
c3a50dc to
2daa3f1
Compare
|
/ok to test 2daa3f1 |
|
/ok to test 380f0c0 |
|
/ok to test ef0241e |
Adds an S3 destination for `dynamo.request.trace.v1` records as a
new `RequestTraceSink` implementation, parallel to the existing
OTLP sink. Same record shape as every other sink -- new destination,
no schema change.
Records are batched in-process as gzipped JSONL and each finished
batch is uploaded via a single `PutObject`. Object keys use a simple
time-based layout for this PR (`{prefix}/{yyyy}/{mm}/{dd}/{host}-
{HHMMSS}-{seq}.jsonl.gz`); richer partitioning (model/day/hour Hive
style), tunable roll thresholds, and Prometheus metrics ship in a
follow-up.
Credentials come from the AWS SDK default provider chain -- env vars,
IMDS, IRSA, Pod Identity, and shared profiles are all handled by the
SDK. How the frontend pod is credentialed is a deployment concern
documented in a separate PR, not this sink's.
The `request-trace-s3` cargo feature is off by default so the AWS
SDK dependency is opt-in. When the feature is off, selecting
`DYN_REQUEST_TRACE_SINKS=...,s3` fails startup with a clear error.
Refs: ai-dynamo#11768 (supersedes ai-dynamo#10903).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Yiqiu Liu <yiqiuliu@amazon.com>
ef0241e to
9b417aa
Compare
|
/ok to test 9b417aa |
|
/ok to test 34c5379 |
|
/ok to test 34c5379 |
|
/ok to test d4a7a9f |
|
/ok to test fedc2da |
|
/ok to test fedc2da |
Summary
Adds a native S3
RequestTraceSinkthat writes existingdynamo.request.trace.v1records directly to a bucket -- parallel to the OTLP sink, no schema change, no partitioning. Smallest reviewable unit for the S3 destination work in #11768.RequestTraceSinkKind::S3variant withS3RequestTraceSink(lib/llm/src/request_trace/s3_sink.rs), gated behind the newrequest-trace-s3cargo feature so the AWS SDK is opt-in.PutObject. Object keys use a simple time-based layout for this PR ({prefix}/{yyyy}/{mm}/{dd}/{host}-{HHMMSS}-{seq}.jsonl.gz).DYN_REQUEST_TRACE_S3_{BUCKET,REGION,PREFIX}; opt in viaDYN_REQUEST_TRACE_SINKS=...,s3.Scope split (per #11768)
This is PR 1 of 3:
model=/date=/hour=keys, tunable roll thresholds, Prometheus metrics.Supersedes #10903, which was pinned to the pre-ai-dynamo/dynamo#11180
auditmodule.Test plan
cargo fmt --check -p dynamo-llmcargo-deny -L error --all-features check licenses bans(bans ok, licenses ok)cargo metadata --locked --format-version 1cargo doc --no-deps-- 0 warnings from new filescargo test --locked --doccargo clippy --no-deps --all-targets -- -D warnings(default features)cargo clippy --no-deps --all-targets --features request-trace-s3 -- -D warningscargo machete-- no unused depscargo test -p dynamo-llm --lib --features request-trace-s3 request_trace-- 66/66 pass, including 4 news3_sinktestsNon-goals (deferred to PRs 2/3 per #11768)
Summary by CodeRabbit