Repository navigation
Athena: harden raw JSONL trace queries - #20
Conversation
Keep immutable JSONL authoritative while defining a schema-compatible Parquet projection and read-only MCP permission. A representative stream-day contains 2,438 chunks and 1.51 GB, so the separate layout enables compaction without moving raw data or changing the reader contract. Validate the empty live table with a bounded Athena query that returns zero rows and scans zero bytes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe PR adds event-time partition indexes, updates event synchronization and storage to use per-day paths, and changes Athena queries to select indexed days and object paths under a shared deadline. It also adds bucket freshness metadata to readiness and CLI status reporting, with updated operational documentation. Event-time storage and indexing
Athena and readiness
Sequence Diagram(s)sequenceDiagram
participant TraceClient
participant Backend
participant event_partitions
participant AwsEventQuery
participant Athena
TraceClient->>Backend: trace request
Backend->>event_partitions: resolve candidate days and objects
event_partitions-->>Backend: day and raw object paths
Backend->>AwsEventQuery: run SQL with shared deadline
AwsEventQuery->>Athena: execute and paginate query
Athena-->>AwsEventQuery: rows or classified outcome
AwsEventQuery-->>Backend: query result and metrics
Backend-->>TraceClient: trace response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@deploy/aws/mcp-reader.yaml`:
- Around line 33-35: Constrain the GlueParquetTable parameter’s AllowedPattern
using the same table-name validation as the Athena stack, preventing `*` and
other values that expand the Glue resource grant beyond one table. Keep the
existing default value unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0dfe9c9-79e4-48a7-a384-76ef1d26f5e2
📒 Files selected for processing (6)
README.mddeploy/aws/athena-trace.yamldeploy/aws/mcp-reader.yamldocs/design.mddocs/parquet-traces.mdsrc/trace_athena.rs
Replace the unused Parquet catalog with event-day and immutable-object range metadata, preserving the existing raw S3 objects while making delayed events queryable through exact Athena path pruning. The live path predicate reduced a representative scan from 7,650,211,000 bytes to 15,660 bytes.\n\nBound multi-query trace requests to one 45-second budget, 1,000 paths, 50,000 rows, and 64 MiB; normalize job ids, prune exact stream identities, expose bucket freshness, and retain mixed-version cursors. Validate 295 default, 274 no-default, and 312 cloud-feature tests plus Helm, HTTP MCP, and CloudFormation checks.
huronat
changed the title
The sole finding is obsolete on 0515cc6: the Parquet parameter, table, and IAM grant were removed. CodeRabbit verified the reported wildcard path no longer applies in discussion_r3661785706.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/readmodel.rs (1)
83-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest that repoint persists publication time.
The fixture-only updates do not exercise the new behavior. Add a scenario test that repoints a temporary model, reads
current.json, and verifiespublished_atis a non-empty RFC3339 timestamp.As per coding guidelines, “Every behavioral change must include a scenario-style unit test based on user expectations.”
🤖 Prompt for 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. In `@src/readmodel.rs` around lines 83 - 90, Add a scenario-style unit test for repoint that uses a temporary model directory, invokes repoint, reads and deserializes current.json, and verifies published_at is non-empty and parses as an RFC3339 timestamp. Keep the test focused on the persisted publication time and follow existing test helpers and setup conventions.Source: Coding guidelines
README.md (1)
215-218: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument both Glue tables, not one.
The PR adds the versioned
trace_events_v1Parquet table while retaining the raw-events table, and grants the MCP role read-only access to both. This wording can lead to incomplete IAM permissions or an incorrect Athena deployment; name both tables and clarify which resourcesdeploy/aws/athena-trace.yamlcreates.🤖 Prompt for 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. In `@README.md` around lines 215 - 218, Update the README description of the MCP-only workload to name both the raw-events table and the versioned trace_events_v1 Parquet table, and state that the MCP role has read-only access to both. Clarify that deploy/aws/athena-trace.yaml creates the external tables and managed-results workgroup without write or crawl operations.
🧹 Nitpick comments (6)
src/trace_athena.rs (5)
483-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmpty object selection returns silently with no metric.
When
selection.pathsis empty the request short-circuits to zero rows without emitting anathena_tracemetrics run, so "nothing selected" is indistinguishable from "never queried" in the[metrics athena_trace]stream. Emitting a run withoutcome=emptykeeps the health surface complete.As per coding guidelines, "Health and quality operations must emit metrics through
metrics::Run".🤖 Prompt for 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. In `@src/trace_athena.rs` around lines 483 - 499, The empty-selection branch in the query method should emit a metrics::Run with outcome=empty before returning zero rows. Update the branch guarded by selection.paths.is_empty() while preserving the existing empty QueryRows result and normal query execution for non-empty selections.Source: Coding guidelines
550-565: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest-only injection branch embedded in production selection.
self.daysfabricates synthetictest.jsonlpaths inside the production code path. It is reachable fromBackend::newonly viaNone, so it is safe today, but the fake-path construction is dead weight in the shipped binary. A#[cfg(test)]query/selection seam (or makingselected_objectsa trait method the tests stub) would keep the production path free of test scaffolding.🤖 Prompt for 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. In `@src/trace_athena.rs` around lines 550 - 565, The selected_objects method embeds test-only synthetic path generation through self.days; move this injection seam behind #[cfg(test)] or replace it with a test-stubbable selection abstraction. Keep production selection free of the days field and test.jsonl fabrication while preserving the existing test behavior through the chosen seam.
138-180: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOnly the deadline path stops the execution.
If
get_query_executionitself errors (throttling, transient network), the loop bails while the Athena execution keeps running and scanning/billing. Routing every early exit from this loop through the samestop_query_executioncall would make cancellation uniform.🤖 Prompt for 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. In `@src/trace_athena.rs` around lines 138 - 180, Update the polling loop around get_query_execution so every early error exit, including polling failures and query/status extraction errors, first attempts to stop the Athena execution through the existing stop_query_execution call. Preserve the current timeout cancellation and error context, and ensure cancellation is also attempted before propagating any failure from the polling path.
1157-1192: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest does not exercise the failing-open case it implies.
stream_pruning_uses_exact_machine_and_canonical_source_suffixesonly covers recognized sources. Add a case with an unrecognizedsource/scope.sourcesvalue to pin down the intended behavior — today it returns every stream, which is the bypass flagged at Lines 523-542.As per coding guidelines, "Every behavioral change must include a scenario-style unit test based on user expectations".
🤖 Prompt for 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. In `@src/trace_athena.rs` around lines 1157 - 1192, The test stream_pruning_uses_exact_machine_and_canonical_source_suffixes only covers recognized sources; add a scenario with an unrecognized source and matching scope.sources to verify selected_streams does not fail open by returning every stream. Assert the expected user-facing result for the unknown source, while preserving the existing recognized-source assertions.Source: Coding guidelines
851-860: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOutcome classification is coupled to error prose.
query_outcomesubstring-matches on message text, so rewording anyensure!/bail!in this file silently reclassifies the metric (e.g. a limit message that drops the word "exceeds" becomeserror). A typed error enum, or at minimum a constant shared by the message and the classifier, would keep the metric stable.🤖 Prompt for 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. In `@src/trace_athena.rs` around lines 851 - 860, Decouple query_outcome from matching arbitrary error prose by introducing stable typed error categories or shared constants for timeout and limit conditions, and use those identifiers both when constructing errors and when classifying outcomes. Update query_outcome and the relevant ensure!/bail! call sites in this module while preserving the existing timeout, limit, and error outcome values.src/event_partitions.rs (1)
265-285: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
bucket_newest_eventlists the entireevent-partitions/prefix, which grows unbounded with day/object-index count.
bucket.list(&format!("{PREFIX}/"))scans every key underevent-partitions/, including onetrack.<day>.jsonobject-index entry per (stream, day) ever recorded (filtered out client-side afterward). This scan cost grows linearly with total historical days across all streams, unlike the doc comment's claim of reading only "small per-stream metadata objects." Since this backs freshness reporting (status/readiness checks), it will get progressively slower for long-lived buckets. Consider tracking the stream set explicitly (e.g. reusing theEVENT_STREAMSregistry already maintained insync.rs) instead of deriving it from a full prefix listing.🤖 Prompt for 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. In `@src/event_partitions.rs` around lines 265 - 285, Update bucket_newest_event to avoid scanning the entire event-partitions prefix; iterate the existing EVENT_STREAMS registry from sync.rs and load each stream’s metadata object directly. Preserve the current newest timestamp comparison and missing-index handling while eliminating client-side filtering of historical day/object-index keys.
🤖 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 `@src/event_partitions.rs`:
- Around line 22-42: The EventTimeRange::record comparisons and related
newest-event selection compare variable-width AutoSi timestamp strings, which
can misorder whole-second and sub-second instants. Parse timestamps into
comparable DateTime values before updating min_ts/max_ts and selecting the
newest event, while preserving the stored timestamp representation and existing
invalid-value behavior. Add a scenario test covering events with both
whole-second and sub-second timestamps.
In `@src/mcp_http.rs`:
- Around line 436-440: Update the health response construction around
bucket_freshness_error so unauthenticated /health and /ready endpoints expose
only a stable generic error indicator instead of freshness.error.to_string().
Keep detailed bucket/provider diagnostics restricted to protected logging, while
preserving the existing freshness fields and response behavior.
In `@src/mcp.rs`:
- Around line 444-459: Update refresh_bucket_freshness to create a metrics::Run
for the freshness refresh operation, recording the outcome, elapsed time,
whether a read-model pointer format is present, and whether raw events are
available; ensure the metrics block is emitted via emit() before returning the
result.
- Around line 450-457: Update the BucketFreshness refresh flow around
bucket_newest_event so a raw-index scanning error does not discard already
parsed current.json pointer metadata. Preserve published_read_model and
read_model_format, record the raw-index failure in the freshness error/partial
metadata, and keep /ready compatible when a valid remote read model exists. Add
a test covering a malformed partition and verifying the preserved metadata and
readiness behavior.
In `@src/sync.rs`:
- Around line 670-706: Bound the per-pull iteration over index.physical_days()
in pull_events_from so historical partitions are not repeatedly listed forever,
while preserving retrieval of late-arriving data through a sealing/pruning
mechanism or equivalent bounded compatibility path. Keep the existing
partition_cursors behavior for active/unsealed days and retain the global
cursors compatibility scan only as needed for older writers.
In `@src/trace_athena.rs`:
- Line 318: Update the caching logic around self.cached = Some(store) so stores
loaded by filtered list operations or without session expansion cannot satisfy
later show/compare lookups. Cache only fully unfiltered, expanded stores, or
mark partial stores and ensure those lookup paths skip them, matching the
existing partial-search safeguard.
- Around line 608-616: The legacy-stream branch in the selection logic currently
adds every physical object before the MAX_OBJECT_PATHS validation, causing
narrow windows to fail. Update the else branch to filter objects using
day_from_event_key against the requested window’s day span before extending
selected, while preserving the non-empty paths requirement and applying the
existing limit check afterward.
- Around line 523-542: The stream filtering logic around streams.retain must
preserve unrecognized source values in normalized lowercase form instead of
discarding them, and normalize stream and requested/scope sources identically so
explicit unknown filters and all-unknown ReadScope entries fail closed. Update
src/trace_athena.rs lines 523-542 accordingly; extend
stream_pruning_uses_exact_machine_and_canonical_source_suffixes at lines
1157-1192 with unrecognized source and scope.sources cases asserting no streams
are returned.
- Around line 566-611: Update the stream-selection logic around
`event_partitions::load` and `bucket.list` so indexed streams derive candidate
days first, then list only `events/{stream}/chunks/track.{day}/` for those days.
Keep the broad stream-prefix listing only when no partition index exists, and
eliminate the per-day `physical.iter().filter(...)` scan by using each day’s
targeted listing results directly. Preserve selection of indexed candidates and
physically present objects missing from the object index.
In `@src/view.rs`:
- Around line 845-862: Add a scenario-style unit test covering a bucket-backed
Status with populated freshness fields. Assert both Markdown and JSON output
include the raw-event time, publication time, read-model format, check time, and
freshness error state, covering the rendering paths around the bucket freshness
output.
---
Outside diff comments:
In `@README.md`:
- Around line 215-218: Update the README description of the MCP-only workload to
name both the raw-events table and the versioned trace_events_v1 Parquet table,
and state that the MCP role has read-only access to both. Clarify that
deploy/aws/athena-trace.yaml creates the external tables and managed-results
workgroup without write or crawl operations.
In `@src/readmodel.rs`:
- Around line 83-90: Add a scenario-style unit test for repoint that uses a
temporary model directory, invokes repoint, reads and deserializes current.json,
and verifies published_at is non-empty and parses as an RFC3339 timestamp. Keep
the test focused on the persisted publication time and follow existing test
helpers and setup conventions.
---
Nitpick comments:
In `@src/event_partitions.rs`:
- Around line 265-285: Update bucket_newest_event to avoid scanning the entire
event-partitions prefix; iterate the existing EVENT_STREAMS registry from
sync.rs and load each stream’s metadata object directly. Preserve the current
newest timestamp comparison and missing-index handling while eliminating
client-side filtering of historical day/object-index keys.
In `@src/trace_athena.rs`:
- Around line 483-499: The empty-selection branch in the query method should
emit a metrics::Run with outcome=empty before returning zero rows. Update the
branch guarded by selection.paths.is_empty() while preserving the existing empty
QueryRows result and normal query execution for non-empty selections.
- Around line 550-565: The selected_objects method embeds test-only synthetic
path generation through self.days; move this injection seam behind #[cfg(test)]
or replace it with a test-stubbable selection abstraction. Keep production
selection free of the days field and test.jsonl fabrication while preserving the
existing test behavior through the chosen seam.
- Around line 138-180: Update the polling loop around get_query_execution so
every early error exit, including polling failures and query/status extraction
errors, first attempts to stop the Athena execution through the existing
stop_query_execution call. Preserve the current timeout cancellation and error
context, and ensure cancellation is also attempted before propagating any
failure from the polling path.
- Around line 1157-1192: The test
stream_pruning_uses_exact_machine_and_canonical_source_suffixes only covers
recognized sources; add a scenario with an unrecognized source and matching
scope.sources to verify selected_streams does not fail open by returning every
stream. Assert the expected user-facing result for the unknown source, while
preserving the existing recognized-source assertions.
- Around line 851-860: Decouple query_outcome from matching arbitrary error
prose by introducing stable typed error categories or shared constants for
timeout and limit conditions, and use those identifiers both when constructing
errors and when classifying outcomes. Update query_outcome and the relevant
ensure!/bail! call sites in this module while preserving the existing timeout,
limit, and error outcome values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 350b97f4-8702-4710-8f2f-de92f8e5b3ee
📒 Files selected for processing (13)
README.mddeploy/aws/mcp-reader.yamldocs/design.mdsrc/event_partitions.rssrc/main.rssrc/mcp.rssrc/mcp_http.rssrc/readmodel.rssrc/sync.rssrc/trace_athena.rssrc/track.rssrc/tui.rssrc/view.rs
💤 Files with no reviewable changes (1)
- deploy/aws/mcp-reader.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/design.md
Fail closed on unknown source scopes, compare event times as parsed instants, preserve compatible pointer readiness on partial freshness failures, and sanitize unauthenticated health diagnostics. Use exact per-day object watermarks to eliminate unchanged historical LIST calls while preserving late older-day discovery. Bound legacy Athena selection to requested physical days and cancel failed polling paths. Validated 299 default, 278 no-default, and 317 full-feature tests plus MCP HTTP smoke, Helm rendering, and the shipped release build.
|
Resolved the remaining outside-diff and nitpick review items in e770fa8 as well:
The old README suggestion to document two Glue tables is obsolete: this PR intentionally removed the Parquet table, parameter, IAM grant, and documentation. The deployment now exposes only synty.raw_events. Validation: 299 default tests, 278 no-default tests, 317 S3/GCS/MCP/Athena tests, MCP HTTP smoke, Helm lint and default/optional rendering, release build, and git diff --check all pass. The AWS templates are unchanged in this commit; local YAML parsing passed, while a redundant live validation retry was unavailable because the local sie SSO session had expired. |
Superseded by e770fa8. All ten inline findings were fixed and CodeRabbit explicitly replied to every thread that the reported issue is addressed. The outside-diff publication-time test and six nitpicks were also implemented; the Parquet README suggestion is obsolete because the Parquet table and grant were removed. Rust and deploy CI are green.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/event_partitions.rs`:
- Around line 119-125: Add a scenario-style unit test in the existing
#[cfg(test)] block for EventPartitionIndex::record_object, verifying that
recording a key advances the per-day object_cursors watermark and that a later
lexicographically smaller key leaves the cursor unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b22f0ea-ca87-4b12-a0d5-f755c5c00bf4
📒 Files selected for processing (9)
README.mddocs/design.mdsrc/event_partitions.rssrc/mcp.rssrc/mcp_http.rssrc/readmodel.rssrc/sync.rssrc/trace_athena.rssrc/view.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/mcp_http.rs
- docs/design.md
- src/view.rs
- README.md
- src/mcp.rs
- src/sync.rs
- src/trace_athena.rs
Add a direct scenario proving that per-day object cursors advance independently and never regress when a late lexicographically smaller key appears. The full s3,gcs,mcp-http,athena matrix passes all 318 tests.
CodeRabbit confirmed the only requested change was addressed in 6042581; dismissing the stale review state while the new-head check completes.
1 participant
Summary
$pathpredicates; unknown legacy objects remain conservative, never invisiblejob:*follow-up lookup, exact machine/source stream pruning, shared 45-second request deadlines, and standard Athena outcome metricsNo raw S3 object is moved, copied, rewritten, or deleted. Existing raw objects
remain queryable without a migration. A metadata-only range backfill is
recommended before broad historical use so legacy capture-day objects can be
pruned efficiently; it is not required for correctness.
Bounds and safety
SELECTstatements onlyValidation
cargo test --locked— 295 passedcargo test --locked --no-default-features— 274 passedcargo test --locked --features s3,gcs,mcp-http,athena— 312 passedcargo build --release --locked$pathquery against one immutable object returned 24 events and scanned 15,660 bytesevent-partitions/and the old derived Parquet prefix were empty before this change; no bucket objects were writtenDeployment
The code commit does not publish or roll out a binary. The existing
CloudFormation stacks can safely be updated from this PR to delete only the
empty
trace_events_v1Glue table and its IAM grant while retainingsynty.raw_events, the bounded workgroup, and the read-only MCP role.Summary by CodeRabbit