Replace Cloudwatch with Athena on the debug page#2762
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds Athena-based trace querying to replace CloudWatch URIs. The backend introduces Athena infrastructure with time-partitioned SQL generation, the debug route wires Athena queries into responses, the frontend adds a clipboard-friendly ChangesAthena Trace Querying
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/querying-logs.md (1)
73-96:⚠️ Potential issue | 🟡 MinorFix Athena reserved-word quoting in
server/querying-logs.mdIn the SQL example (lines 73-96),
timestamp/year-minuteare left unquoted, but the text advises backticks for reserved words. In Athena, reserved identifiers inSELECT/queries must be escaped with double quotes ("timestamp","year", etc.); backticks are for DDL. Also,LIMIT/OFFSET/timeoutin your example are query keywords (not column names), so the reserved-word backtick guidance should only apply if those are actual column identifiers.🤖 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 `@server/querying-logs.md` around lines 73 - 96, Update the SQL example to use Athena-compatible identifier quoting: replace backtick guidance with double quotes for reserved identifiers (e.g., use "timestamp", "year", "month", "day", "hour", "minute" in the SELECT and WHERE), and clarify that SQL keywords like LIMIT, OFFSET, and timeout are not column identifiers and should not be quoted; adjust the paragraph text to state double quotes are required for reserved identifiers in Athena and that quoting guidance only applies to actual column names such as timestamp/year-minute.
🧹 Nitpick comments (1)
client/www/pages/debug-uri/[trace-id]/[span-id].tsx (1)
20-26: 💤 Low valueConsider logging clipboard errors for debugging.
The
.catch(() => {})silently swallows clipboard write errors. While this prevents user-facing errors when clipboard access is denied, logging the error could help diagnose issues in development or production monitoring.Optional logging enhancement
navigator.clipboard .writeText(query) .then(() => { setCopied(true); setTimeout(() => setCopied(false), 1500); }) - .catch(() => {}); + .catch((err) => { + console.warn('Failed to copy to clipboard:', err); + });🤖 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 `@client/www/pages/debug-uri/`[trace-id]/[span-id].tsx around lines 20 - 26, The clipboard write promise currently swallows errors in navigator.clipboard.writeText(query).catch(() => {}); update the catch to accept the error and log it (e.g., console.error or your app logger) with context (include the query and identifiers if available) so failures are visible for debugging, while still preventing user-facing errors; leave the existing setCopied(true) and timeout behavior unchanged.
🤖 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 `@server/querying-logs.md`:
- Around line 153-170: Update the minute-window example around the read_parquet
snippet to explicitly handle hour/day rollovers: explain that when you list “5
before to 5 after” you must compute minute timestamps across hour and midnight
boundaries (referencing the "Trace ID → partition" decoding step) and include
paths for the previous/next hour/day as needed (e.g., generate explicit s3 paths
for minutes that fall in hour-1, hour, and hour+1 or across date change), and
add a short helper note or pseudocode to produce those explicit paths so readers
don’t miss files at HH:00 or midnight when using read_parquet and filtering by
trace_id.
---
Outside diff comments:
In `@server/querying-logs.md`:
- Around line 73-96: Update the SQL example to use Athena-compatible identifier
quoting: replace backtick guidance with double quotes for reserved identifiers
(e.g., use "timestamp", "year", "month", "day", "hour", "minute" in the SELECT
and WHERE), and clarify that SQL keywords like LIMIT, OFFSET, and timeout are
not column identifiers and should not be quoted; adjust the paragraph text to
state double quotes are required for reserved identifiers in Athena and that
quoting guidance only applies to actual column names such as
timestamp/year-minute.
---
Nitpick comments:
In `@client/www/pages/debug-uri/`[trace-id]/[span-id].tsx:
- Around line 20-26: The clipboard write promise currently swallows errors in
navigator.clipboard.writeText(query).catch(() => {}); update the catch to accept
the error and log it (e.g., console.error or your app logger) with context
(include the query and identifiers if available) so failures are visible for
debugging, while still preventing user-facing errors; leave the existing
setCopied(true) and timeout behavior 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: faa09b2e-3507-4471-a1c6-b8dbd5cd72b1
📒 Files selected for processing (4)
client/www/pages/debug-uri/[trace-id]/[span-id].tsxserver/querying-logs.mdserver/src/instant/dash/routes.cljserver/src/instant/util/tracer.clj
Contributor
|
View Vercel preview at instant-www-js-athena-debug-jsv.vercel.app. |
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
2 participants
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.
Replaces the link to a cloudwatch search for the traceid with a link to athena.
Athena doesn't let you include a query in the url, so I put the query in a select box beneath and also copy it to your clipboard when you click the link.
I also improved the querying-logs.md file with some improvements after going on a debugging session with claude.
This is what it looks likes when you're logged in as an admin: