Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new v1→v2 analytics migration guide MDX, inserts an "Analytics Endpoints" card into the v1 migration index, and registers the new page in the docs manifest. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
chronark
requested review from
Flo4604,
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/docs/api-reference/v1/migration/analytics.mdx (2)
185-248: Add descriptive labels to code blocks for consistency.The Common Migration Patterns section uses unlabeled code blocks, while earlier sections (like Access and Authentication and Request Format Changes) include descriptive labels (e.g., "v1 Request Structure", "v2 Response Structure"). Add labels to all code blocks for consistency and clarity.
Update code blocks to match the labeling pattern used elsewhere:
- ```bash + ```bash v1: Basic Query curl -X GET "https://api.unkey.dev/v1/analytics.getVerifications?apiId=api_123&start=1620000000000&groupBy=day"V2: SQL Equivalent
Apply the same pattern to the remaining examples: Filter by User, Outcome Breakdown, and Top Users by Usage. --- `252-264`: **Standardize link formats in Getting Help section.** The links in Getting Help mix absolute and relative URLs inconsistently: - Line 254: `https://unkey.com/docs/analytics` (absolute) - Lines 255–260: `/analytics/...` (relative) - Line 261: `https://unkey.com/docs/api-reference/v1/migration` (absolute) Compare this to the Getting Help section in index.mdx (lines 119-125), which uses consistent absolute URLs for all external and internal links. For maintainability, standardize all links in this section to use either: 1. **All absolute URLs** (recommended for consistency with the migration index): `https://unkey.com/docs/analytics`, `https://unkey.com/docs/api-reference/v1/migration` 2. **Or all relative links** for internal docs (if that's a project convention) Choose one approach and apply it consistently. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6a3148c3484598966be17243eb3755e394d820a1 and f213a56d491150b242470a51da4f5924129e8b43. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `apps/docs/api-reference/v1/migration/analytics.mdx` (1 hunks) * `apps/docs/api-reference/v1/migration/index.mdx` (1 hunks) * `apps/docs/docs.json` (1 hunks) </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: Analyze (javascript-typescript) </details> <details> <summary>🔇 Additional comments (6)</summary><blockquote> <details> <summary>apps/docs/api-reference/v1/migration/index.mdx (1)</summary><blockquote> `110-112`: **LGTM!** The new Analytics card is well-placed, properly formatted, and consistent with other migration category cards. The icon and description clearly communicate the purpose. </blockquote></details> <details> <summary>apps/docs/api-reference/v1/migration/analytics.mdx (4)</summary><blockquote> `1-21`: **LGTM!** Clear, well-structured introduction to the migration guide with good context on the v1-to-v2 transition. --- `24-80`: **LGTM!** Clear explanation of access changes from automatic v1 to opt-in v2. Permission table is well-organized and the private beta context is helpful for users. --- `84-176`: **LGTM!** Request and response format sections are clear with good examples showing v1-to-v2 transformation. The automatic security filtering explanation is valuable. --- `71-79`: **Clarify apiId vs key_space_id terminology.** Line 79 states "you can filter by `apiId` in your query," but all SQL examples use `key_space_id` instead. This is confusing for users migrating from v1 to v2: - V1 examples (line 98, 203, 220, 237) use `apiId` as a parameter - V2 examples (lines 195, 212, 229, 246) use `key_space_id` in SQL WHERE clauses - Line 79 mentions `apiId` as a filterable field Please clarify the relationship between these fields: 1. Are `apiId` and `key_space_id` the same thing with different names (v1 vs v2)? 2. Or are they different fields that need different handling? If they're the same, consider updating line 79 to explicitly say "you can filter by `key_space_id` (the v2 equivalent of `apiId`)" or update the note to mention both names. If they're different, add an explanation of the mapping. Also applies to: 195-195, 212-212, 229-229, 246-246 </blockquote></details> <details> <summary>apps/docs/docs.json (1)</summary><blockquote> `309-309`: **LGTM!** The analytics entry is correctly placed in the migration guides list with proper JSON formatting and sequencing. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
perkinsjr
left a comment
perkinsjr
left a comment
There was a problem hiding this comment.
LGTM.
EXTREMELY minor nit, maybe we make the opt in a warning? This double info block is fine and can go to prod.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/docs/api-reference/v1/migration/analytics.mdx (1)
106-113: Consider clarifying the bash quote escaping syntax.The pattern
'\''ks_123'\''for escaping quotes within bash single-quoted strings is technically correct but may confuse users. Consider adding a brief comment or showing the raw JSON structure separately for clarity.You could add a note like:
# Raw JSON payload (before bash escaping): # {"query": "SELECT time, SUM(count) as total FROM key_verifications_per_day_v1 WHERE key_space_id = 'ks_123' ..."} # Bash command with escaped quotes: curl -X POST https://api.unkey.com/v2/analytics.getVerifications ...Alternatively, keep the current examples but add inline comments explaining the escaping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/docs/api-reference/v1/migration/analytics.mdx(1 hunks)
🔇 Additional comments (6)
apps/docs/api-reference/v1/migration/analytics.mdx (6)
1-4: Front matter looks good.Metadata is correctly formatted and descriptive for the guide.
6-21: Clear, focused overview.Effectively frames the v1→v2 transition with key changes and benefits. Good foundation for the detailed sections that follow.
121-176: Response format section is clear and well-structured.The comparison effectively shows the shift from fixed V1 schema to flexible V2 SQL-based responses. The meta/data envelope pattern is clearly illustrated with the dynamic fields note.
252-263: Verify all documentation links resolve correctly and consider URL consistency.The help section mixes absolute URLs (line 254) and relative URLs (lines 255-260). While this may be intentional, ensure:
- The absolute URL
https://unkey.com/docs/analyticsis correct and up-to-date- All relative paths
/analytics/getting-started,/analytics/schema-reference, etc. exist and are correct- Consistency in URL format (standardize to all relative or all absolute where possible)
26-69: All links and endpoints verified — no issues found.The documentation links and endpoint URLs are correct and properly configured:
- Endpoints: v1 uses
api.unkey.dev, v2 usesapi.unkey.com(confirmed throughout the file)- Documentation links: All relative links (
/analytics,/analytics/getting-started,/analytics/schema-reference, etc.) are properly registered inapps/docs/docs.jsonand resolve correctly- Permissions: The permission names
api.*.read_analyticsandapi.{apiId}.read_analyticsmatch the actual v2 permissions system (confirmed inpackages/rbac/src/permissions.ts)
180-248: All migration patterns verified as correct.Field names (
key_space_id,external_id,outcome,time,count), table name (key_verifications_per_day_v1), outcome values (VALID,RATE_LIMITED,INVALID), and SQL functions (sumIf,SUM,GROUP BY,ORDER BY,LIMIT) are all consistent with the analytics schema and working examples inquery-examples.mdx. The migration patterns accurately represent V1-to-V2 migration scenarios.
|
yes |
2 participants
What does this PR do?
Add a comprehensive analytics migration guide for transitioning from v1 to v2 analytics system. The guide explains the shift from predefined parameters to SQL-based querying, covering key changes in access permissions, request formats, response structures, and common migration patterns.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated