Skip to content

feat(request-trace): native S3 sink for request-trace records - #1

Closed
chethanuk wants to merge 320 commits into
mainfrom
fix/issue-11768-featrequesttrace-native-s3-sink-for-requesttrace
Closed

feat(request-trace): native S3 sink for request-trace records#1
chethanuk wants to merge 320 commits into
mainfrom
fix/issue-11768-featrequesttrace-native-s3-sink-for-requesttrace

Conversation

@chethanuk

Copy link
Copy Markdown
Owner

Overview:

Add a native S3 RequestTraceSink so deployments can write dynamo.request.trace.v1 records straight to a bucket without an OpenTelemetry collector hop. Same schema as existing file/OTel/NATS sinks; new destination only.

Details:

  • Extract SegmentSink from JsonlGzipWriter so roll/shutdown can finalize segments that cannot be appended after create (S3 objects).
  • Add RequestTraceSinkKind::S3 + S3RequestTraceSink / S3SegmentSink, gated by default-on request-trace-s3.
  • Object keys: {prefix}/YYYY/MM/DD/HH/{instance}-{startup}-{seq:06}.jsonl.gz.
  • Env: DYN_REQUEST_TRACE_SINKS=s3, DYN_REQUEST_TRACE_S3_{BUCKET,REGION,PREFIX,ROLL_BYTES,FLUSH_INTERVAL_MS}.
  • Credentials: AWS SDK default provider chain (no operator/CRD changes).
  • Docs: docs/observability/request-tracing.md.
  • Tests: unit replay (StaticReplayClient) + lib/llm/tests/request_trace_s3_replay.rs.

Where should the reviewer start?

  1. lib/llm/src/telemetry/jsonl_gz.rsSegmentSink trait + roll/shutdown await close
  2. lib/llm/src/telemetry/s3_segment_sink.rs — PutObject segment sink
  3. lib/llm/src/request_trace/sink.rsS3RequestTraceSink + tests
  4. lib/llm/src/request_trace/config.rs — kind parse + policy fields

Related Issues

🔗 This PR is linked to an issue:

How to test

cargo test -p dynamo-llm --lib --no-default-features --features request-trace-s3 request_trace::
cargo test -p dynamo-llm --lib --no-default-features --features request-trace-s3 telemetry::jsonl_gz::
cargo test -p dynamo-llm --test request_trace_s3_replay --no-default-features --features request-trace-s3
cargo check -p dynamo-llm --no-default-features
cargo clippy -p dynamo-llm --lib --no-default-features --features request-trace-s3 -- -D warnings

Note on competing work

Upstream already has open PR ai-dynamo#11806 (YiqiuLiu) for the same issue. This fork PR is our independent implementation (SegmentSink refactor + roll-path close + StaticReplayClient tests). Do not open a second upstream PR without human review of the overlap.

sttts and others added 30 commits July 3, 2026 17:55
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
… images (ai-dynamo#11198)

Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: xianlubird <xianlubird@gmail.com>
…amo#10986)

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Ryan McCormick <rmccormick@nvidia.com>
Co-authored-by: Dan Gil <dagil@nvidia.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
…1110)

Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…ntime::shutdown() (ai-dynamo#11068)

Signed-off-by: nnshah1 <neelays@nvidia.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
….33 in /deploy/snapshot (ai-dynamo#11238)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
…mo#11211)

Signed-off-by: Wenxin Zhang <wenxin.zhang@intel.com>
Signed-off-by: wenxin.zhang <wenxin.zhang@intel.com>
…i-dynamo#11200)

Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
Co-authored-by: Yuewei Na <nv-yna@users.noreply.github.com>
Signed-off-by: Po-Han Huang <pohanh@nvidia.com>
Signed-off-by: Matej Kosec <mkosec@nvidia.com>
ai-dynamo#11201)

Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: Graham King <grahamk@nvidia.com>
…ynamo#11046)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
PeaBrane and others added 13 commits July 16, 2026 18:01
…11440)

Signed-off-by: Tushar Sharma <tusharma@nvidia.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
)

Signed-off-by: Alexandre Milesi <milesial@users.noreply.github.com>
Signed-off-by: milesial <milesial@users.noreply.github.com>
…i-dynamo#11727)

Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Adds aws-sdk-s3/aws-config as optional dependencies gated by a new
request-trace-s3 feature, and aws-smithy-http-client (test-util) as a
dev-dependency so the sink can be tested against replayed HTTP responses
instead of a live bucket.

The feature is default-on: the published wheel must link it or
DYN_REQUEST_TRACE_SINKS=s3 cannot work at runtime. Builds that do not
need it can opt out with --no-default-features.

Versions match lib/kvbm-engine/Cargo.toml so the workspace resolves a
single major across all three lockfiles.

Landing the feature before the tests keeps the upcoming red tests from
being silently compiled out.

Refs ai-dynamo#11768

Signed-off-by: Hermes Agent <hermes@nousresearch.com>
Covers the reported gap through the entry point an operator actually
uses -- the DYN_REQUEST_TRACE_SINKS env var -- plus the sink behaviour
that makes the destination usable:

- parses_s3_sink_name: table-driven over the sink-name grammar
  (case, whitespace, dedup, combination with other sinks, unknown
  names). Today "s3" hits the unknown-sink warn arm and is dropped.
- s3_policy_defaults_load_from_env / s3_policy_reads_overrides_from_env:
  env -> RequestTracePolicy, including that an s3-only config does not
  default a local file path.
- s3_sink_uploads_pending_records_on_shutdown: roll thresholds are set
  high and the flush interval long, so only shutdown() can trigger the
  upload. Asserts one PUT, the object key layout, and that the gzipped
  body carries the same record shape as the file sink.
- s3_sink_uploads_each_segment_on_roll: S3 objects cannot be appended
  to, so every roll must finalize the segment it leaves behind.
- s3_sink_without_bucket_fails: missing bucket must name the variable.

All fail to compile: RequestTraceSinkKind has no S3 variant, there is no
S3RequestTraceSink or s3_segment_sink module, and the env constants do
not exist.

Refs ai-dynamo#11768

Signed-off-by: Hermes Agent <hermes@nousresearch.com>
Make roll/shutdown finalize segments through SegmentSink so destinations
that cannot be appended after creation (S3 objects) can commit on close.
send/shutdown take &self via short-lived Mutex guards so trait-object
sinks can flush without holding a lock across await.

No intended behavior change for the existing file sink or fpm_trace.

Refs: ai-dynamo#11768
Signed-off-by: ChethanUK <chethanuk@outlook.com>
Add RequestTraceSinkKind::S3 so the frontend can PUT rolled
dynamo.request.trace.v1 .jsonl.gz segments straight to a bucket without
an OpenTelemetry collector hop. Uses the AWS SDK default credential
chain; opt in with DYN_REQUEST_TRACE_SINKS=s3 and DYN_REQUEST_TRACE_S3_BUCKET.

Object keys: {prefix}/YYYY/MM/DD/HH/{instance}-{startup}-{seq}.jsonl.gz.
Roll and final close await close_segment so pending records are not
dropped. Behind default-on request-trace-s3 feature.

Refs: ai-dynamo#11768
Signed-off-by: ChethanUK <chethanuk@outlook.com>
Mirror the in-module StaticReplayClient coverage under
lib/llm/tests/request_trace_s3_replay.rs so CI and stage gates see a
dedicated test path for the native S3 sink.

Refs: ai-dynamo#11768
Signed-off-by: ChethanUK <chethanuk@outlook.com>
@codeant-ai

codeant-ai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Skipping CodeAnt AI review — this PR changes more than 100 files, which usually means a migration, codemod, or vendored drop. Line-level review on diffs this large produces duplicate findings on the same rewrite pattern and drowns out anything that actually matters.

If you still want a review, comment @codeant-ai : review. For better signal, consider splitting the PR into smaller chunks.

@github-advanced-security github-advanced-security AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces significant updates to the Dynamo project, including improvements to the CI/CD pipeline, the introduction of a structured DGD (DynamoGraphDeployment) materialization and override protocol, and enhancements to the multimodal request handling and profiler. The review comments identify a missing import in sglang_processor.py, a potential crash in codeowners_match.py due to unhandled subprocess errors, and a missing finite check for the prefill_threshold in router_args.py. All review comments provide actionable feedback and should be addressed.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

from dataclasses import dataclass
from typing import Any

from sglang.srt.parser.conversation import chat_template_exists

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The copy module is used on line 647 (schema = copy.deepcopy(schema)) but it is not imported in this file. Please import copy to avoid a NameError at runtime.

Suggested change
from sglang.srt.parser.conversation import chat_template_exists
import copy
from sglang.srt.parser.conversation import chat_template_exists

Comment on lines +256 to +259
def load_tree(repo: Path) -> list[str]:
"""Return tracked files under ``repo`` via ``git ls-files``."""
out = subprocess.check_output(["git", "-C", str(repo), "ls-files"], text=True)
return [p for p in out.splitlines() if p.strip()]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If repo is not a valid git repository, subprocess.check_output will raise an unhandled subprocess.CalledProcessError and crash the execution. Consider wrapping the call in a try-except block to gracefully handle the error and exit with a clear message, similar to the implementation in changed_paths.

Suggested change
def load_tree(repo: Path) -> list[str]:
"""Return tracked files under ``repo`` via ``git ls-files``."""
out = subprocess.check_output(["git", "-C", str(repo), "ls-files"], text=True)
return [p for p in out.splitlines() if p.strip()]
def load_tree(repo: Path) -> list[str]:
"""Return tracked files under ``repo`` via ``git ls-files``."""
try:
out = subprocess.check_output(["git", "-C", str(repo), "ls-files"], text=True)
except subprocess.CalledProcessError as err:
raise SystemExit(f"git ls-files failed in {repo!r}: {err}") from err
return [p for p in out.splitlines() if p.strip()]

Comment on lines +92 to +94
prefill_threshold = self.active_prefill_tokens_threshold
if prefill_threshold is not None and prefill_threshold < 0:
raise ValueError("--active-prefill-tokens-threshold must be >= 0")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For defensive programming, check math.isfinite on prefill_threshold to ensure it is a valid finite number, preventing non-finite values (like NaN or Infinity) from bypassing the < 0 check.

Suggested change
prefill_threshold = self.active_prefill_tokens_threshold
if prefill_threshold is not None and prefill_threshold < 0:
raise ValueError("--active-prefill-tokens-threshold must be >= 0")
prefill_threshold = self.active_prefill_tokens_threshold
if prefill_threshold is not None and not (
math.isfinite(prefill_threshold) and prefill_threshold >= 0
):
raise ValueError("--active-prefill-tokens-threshold must be a finite value >= 0")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.