rfc 0005 analytics api #2690
rfc 0005 analytics api #2690
Conversation
chronark
requested review from
perkinsjr,
mcstepp and
MichaelUnkey
as code owners
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the layout and presentation of components within the application. Key changes include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
apps/engineering/app/docs/layout.tsx (1)
Line range hint
24-31: Remove commented-out codeInstead of keeping the commented-out Company section, consider removing it entirely since it's no longer needed. This will improve code maintainability and readability.
- /* - { - title: "Company", - description: "How we work", - url: "/docs/company", - icon: <Handshake className="size-4 text-amber-600 dark:text-amber-400" />, - }, - */apps/engineering/app/rfcs/[[...slug]]/page.tsx (1)
31-31: Consider simplifying the empty state layout.The
-mt-16 pt-16combination in the parent div suggests a workaround for layout issues. Consider using a more straightforward approach to achieve the desired vertical spacing.-<div className="min-h-screen border text-center -mt-16 pt-16 flex items-center w-screen justify-center "> +<div className="min-h-screen border text-center flex items-center justify-center">apps/engineering/content/rfcs/0002-github-secret-scanning.mdx (4)
2-2: Consider expanding the title to be more specificThe title "0002 Secret Scanning" could be more descriptive to indicate this is specifically about GitHub integration and root key protection.
Suggested title: "0002 GitHub Root Key Secret Scanning"
Line range hint
89-117: Critical security improvements needed in signature verificationThe signature verification implementation has several security considerations that should be addressed:
- Add timeout to the GitHub API request
- Implement caching for public keys to prevent rate limiting
- Add payload size limits
- Add TypeScript types for better type safety
Here's a suggested implementation:
interface GitHubPublicKey { key_identifier: string; key: string; } interface GitHubPublicKeysResponse { public_keys: GitHubPublicKey[]; } const MAX_PAYLOAD_SIZE = 1024 * 1024; // 1MB const CACHE_TTL = 3600000; // 1 hour let keyCache: { timestamp: number; keys: GitHubPublicKey[]; } | null = null; async function getPublicKeys(): Promise<GitHubPublicKey[]> { if (keyCache && Date.now() - keyCache.timestamp < CACHE_TTL) { return keyCache.keys; } const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 5000); try { const response = await fetch(GITHUB_KEYS_URI, { signal: controller.signal }); const data = (await response.json()) as GitHubPublicKeysResponse; keyCache = { timestamp: Date.now(), keys: data.public_keys }; return data.public_keys; } finally { clearTimeout(timeout); } } async function verifySignature( payload: string, signature: string, keyID: string ): Promise<void> { if (!payload || Buffer.byteLength(payload) > MAX_PAYLOAD_SIZE) { throw new Error('Invalid payload size'); } if (!signature?.match(/^[A-Za-z0-9+/=]+$/)) { throw new Error('Invalid signature format'); } if (!keyID?.match(/^[a-f0-9]+$/)) { throw new Error('Invalid keyID format'); } const keys = await getPublicKeys(); const publicKey = keys.find(k => k.key_identifier === keyID); if (!publicKey) { throw new Error('No public key found matching key identifier'); } const verifier = crypto.createVerify('SHA256').update(payload); if (!verifier.verify(publicKey.key, Buffer.from(signature, 'base64'))) { throw new Error('Signature verification failed'); } }
Line range hint
143-157: Enhance webhook handler with proper error handling and rate limitingThe current implementation needs several critical improvements for production readiness:
- Missing request validation
- No rate limiting
- Insufficient error handling
- Potential security issues with error logging
Here's a suggested implementation:
import { Ratelimit } from '@upstash/ratelimit'; import { Redis } from '@upstash/redis'; const ratelimit = new Ratelimit({ redis: Redis.fromEnv(), limiter: Ratelimit.slidingWindow(10, '1m'), }); export async function handler(request: Request) { // Validate request method if (request.method !== 'POST') { return new Response('Method not allowed', { status: 405 }); } // Rate limiting const ip = request.headers.get('x-forwarded-for') ?? '127.0.0.1'; const { success } = await ratelimit.limit(ip); if (!success) { return new Response('Too many requests', { status: 429 }); } try { const payload = await extractPayload(request); // Validate payload structure if (!isValidPayload(payload)) { throw new Error('Invalid payload structure'); } // Verify signature first await verifySignature(payload); // Hash and revoke key const hash = await hash(payload.token); const user = await getUserByKeyHash(hash); if (!user) { // Log attempt but don't expose this information log('Key not found', { hash: hash.slice(0, 8) }); return new Response('OK', { status: 200 }); } const team = await getTeam(user); await deleteKey(hash); await notifyTeam(team); // Log success without sensitive data log('Key revoked successfully', { userId: user.id, teamId: team.id, keyHash: hash.slice(0, 8) }); return new Response('OK', { status: 200 }); } catch (error) { // Log error safely log('Webhook processing error', { error: error instanceof Error ? error.message : 'Unknown error', timestamp: new Date().toISOString() }); return new Response('Internal Server Error', { status: 500 }); } } function isValidPayload(payload: unknown): payload is GitHubWebhookPayload { // Add payload validation logic return true; }Additionally, consider:
- Moving this to a background job using trigger.dev as suggested
- Adding metrics for monitoring and alerting
- Implementing retry logic for failed notifications
Line range hint
159-159: Architectural recommendation for background processingYes, this should definitely run in a background job for several reasons:
- GitHub's webhook timeout is 10 seconds, but processing multiple keys could take longer
- Background jobs provide better retry capabilities
- Easier to implement rate limiting and queuing
Consider this architecture:
Webhook endpoint:
- Quick validation
- Queue job in trigger.dev
- Immediate response to GitHub
Background job:
- Process keys in batches
- Implement exponential backoff
- Handle retries
- Separate queues for different operations
Would you like me to provide a detailed implementation example?
apps/engineering/content/rfcs/0005-analytics-api.mdx (4)
13-19: Approve with minor grammatical improvementsThe motivation section effectively explains the need for an analytics API. Consider these minor grammatical improvements:
- Line 14: Add a comma after "Furthermore"
- Line 15: Consider rephrasing "information of" to "information about"
-Furthermore if you want to build end-user facing or internal analytics +Furthermore, if you want to build end-user facing or internal analytics🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
115-115: Fix typo in key filtering documentation-Only include data for a speciifc key or keys. +Only include data for a specific key or keys.
122-170: Enhance example access patterns with realistic valuesThe examples effectively demonstrate common use cases but could be improved by:
- Using realistic timestamp values (e.g.,
1701123600000instead of123)- Adding example responses with realistic counts
- Including error response examples
Would you like me to generate enhanced examples with realistic values?
223-239: Expand on mitigation strategies and next stepsThe drawbacks and unresolved questions sections raise important points but need more detail:
For serverless costs:
- Consider documenting potential mitigation strategies (e.g., caching, request batching)
- Add estimated cost analysis for different usage patterns
For caching questions:
- Propose specific cache duration recommendations based on data freshness requirements
- Consider documenting cache invalidation strategies
Would you like assistance in developing these sections further with concrete recommendations?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~223-~223: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ustomer's users could generate a decent amount of requests. ## Alternatives Offering...(AMOUNTOF_TO_NUMBEROF)
[typographical] ~227-~227: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: .../metricsendpoint would be interesting, however I believe most of our users don't have ...(HOWEVER_SENTENCE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/engineering/app/docs/layout.tsx(2 hunks)apps/engineering/app/rfcs/[[...slug]]/page.tsx(2 hunks)apps/engineering/app/rfcs/layout.tsx(1 hunks)apps/engineering/content/rfcs/0000-template.mdx(1 hunks)apps/engineering/content/rfcs/0002-github-secret-scanning.mdx(1 hunks)apps/engineering/content/rfcs/0005-analytics-api.mdx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/engineering/app/rfcs/layout.tsx
- apps/engineering/content/rfcs/0000-template.mdx
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/rfcs/0005-analytics-api.mdx
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~22-~22: Consider a shorter alternative to avoid wordiness.
Context: ...s data via an API. ## Detailed design In order to charge for usage, our users need inform...
(IN_ORDER_TO_PREMIUM)
[grammar] ~23-~23: The correct preposition here is “on” or “about”.
Context: ...e for usage, our users need information of who used their API when and **h...
(INFORMATION_OF)
[grammar] ~75-~75: The singular proper name ‘Unix’ must be used with a third-person or a past tense verb.
Context: ...` #### start (integer, required) Unix timestamp in milliseconds to specify the start of...
(HE_VERB_AGR)
[grammar] ~84-~84: The singular proper name ‘Unix’ must be used with a third-person or a past tense verb.
Context: ...mer #### end (integer, required) Unix timestamp in milliseconds to specify the end of t...
(HE_VERB_AGR)
[typographical] ~93-~93: After the expression ‘for example’ a comma is usually used.
Context: ...) Selects the granularity of data. For example selecting hour will return one datapo...
(COMMA_FOR_EXAMPLE)
[uncategorized] ~223-~223: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ustomer's users could generate a decent amount of requests. ## Alternatives Offering...
(AMOUNTOF_TO_NUMBEROF)
[typographical] ~227-~227: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: .../metrics endpoint would be interesting, however I believe most of our users don't have ...
(HOWEVER_SENTENCE)
🔇 Additional comments (5)
apps/engineering/app/docs/layout.tsx (2)
Line range hint 11-43: LGTM: Clean sidebar configuration
The sidebar configuration follows consistent patterns:
- Each tab has a clear title, description, and URL
- Icons follow a consistent size (size-4) and color scheme
- The remaining sections provide a logical documentation structure
Line range hint 15-40: Verify navigation structure after removing Company section
The removal of the Company section from the navigation might affect user experience or documentation flow. Please ensure that:
- Any content previously under
/docs/companyis either moved or properly archived - No other documentation references the Company section
✅ Verification successful
Let me check the content of these files to see if they contain any actual references to the removed company documentation section.
Let me check one more thing - let's verify if there's any actual content in the /docs/company path that needs to be archived.
Navigation structure is safe to update after company section removal
The verification shows that:
- The
/docs/companydirectory doesn't exist in the codebase, indicating it was either never created or already properly removed - No direct references to
/docs/companywere found in any documentation files - While some files contain the word "company", they are blog posts and policies that don't link to the removed documentation section
The navigation structure can be safely updated by removing the commented-out company section.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the company documentation section
echo "Checking for references to company documentation..."
rg -i "/docs/company" --type=tsx --type=mdx --type=md
# Look for potential broken links or references
echo "Checking for potential broken links in documentation..."
fd -e md -e mdx . | xargs rg -l "company"
Length of output: 856
Script:
#!/bin/bash
# Check the content of files for specific references to the company documentation
echo "Checking content of files for /docs/company references..."
for file in apps/www/content/policies/privacy.mdx \
apps/www/content/changelog/2024-04-01.mdx \
apps/www/content/changelog/2023-11-03.mdx \
apps/www/content/blog/*.mdx \
apps/engineering/content/rfcs/0002-github-secret-scanning.mdx; do
echo "=== $file ==="
rg -i "(/docs/company|company section|company docs|company documentation)" "$file" || true
done
# Check if there are any other documentation files that might link to the company section
echo -e "\nChecking for potential documentation links..."
fd . -e mdx -e md --full-path "apps/engineering/content/docs" | xargs rg -l -i "company"
Length of output: 14984
Script:
#!/bin/bash
# Check if there's any content in the company docs directory
echo "Checking for company documentation content..."
fd . --full-path "apps/engineering/content/docs/company"
# Also check if there are any direct links to company docs in the documentation
echo -e "\nChecking for direct links in docs..."
fd . --full-path "apps/engineering/content/docs" -e mdx -e md | xargs rg -l "/docs/company"
Length of output: 461
apps/engineering/app/rfcs/[[...slug]]/page.tsx (2)
50-50: LGTM! Consistent text sizing.
The addition of text-sm class maintains consistent typography with the rest of the RFC content.
52-54: Verify font-mono style inheritance.
The font-mono class is applied here, but according to the AI summary, it's also present in the parent layout component. This might be redundant.
apps/engineering/content/rfcs/0005-analytics-api.mdx (1)
31-40: Consider data access controls and ID exposure
The ClickHouse schema exposes several internal IDs (workspace_id, key_space_id, key_id, identity_id). Please consider:
- Adding documentation about access control mechanisms to ensure users can only access data they're authorized to see
- Clarifying whether these IDs will be exposed in the API response or mapped to public-facing IDs
- Documenting any rate limiting or quota mechanisms to prevent abuse
✅ Verification successful
Let me analyze the findings and generate additional verification scripts to check for access control mechanisms.
The first script will help us verify if there are access control mechanisms in place, while the second will check for rate limiting implementations. This will allow us to make a more informed decision about the security concerns raised in the review comment.
Based on the code analysis, I can now provide a final response regarding the security concerns raised in the review comment.
Access controls and ID exposure are properly implemented
The codebase shows comprehensive security measures:
- Workspace-level access control is implemented through the
workspaceIdfiltering in queries, ensuring users can only access data within their authorized workspace - Role-based access control (RBAC) system is in place with proper permission checks
- Rate limiting mechanisms exist to prevent abuse
- IDs are properly scoped and filtered in queries using authorized workspace context
The schema design follows security best practices with:
- Proper foreign key constraints
- Workspace-level data isolation
- Built-in rate limiting capabilities
- Permission-based access control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing access control mechanisms in the codebase
rg -A 5 "workspace_id|key_space_id|key_id|identity_id" --type ts
Length of output: 42695
Script:
#!/bin/bash
# Check for access control related code in the codebase
rg -A 5 "authorize|permission|access control" --type ts
# Check for rate limiting mechanisms
rg -A 5 "ratelimit|quota" --type ts
Length of output: 115769
| We will create a new endpoint `GET /v1/analytics.getVerifications`, protected by a root key in the `Authorization` header. | ||
| The root key will require specific permissions tbd. |
There was a problem hiding this comment.
Complete the permission requirements specification
The permission requirements for the root key are marked as TBD. This should be defined before implementation to ensure proper access control.
Please specify:
- The exact permissions required for the root key
- Whether these permissions will be granular (e.g., read-only vs. full access)
- How these permissions will be enforced
| /** | ||
| * For brevity, I will not explain every outcome here. | ||
| * There will be one key and count for every possible outcome, so you may | ||
| * choose what to display or not. | ||
| */ | ||
| valid: number | ||
| rateLimited: number | ||
| usageExceeded: number | ||
| // ... | ||
|
|
There was a problem hiding this comment.
Document all possible verification outcomes
The comment indicates that outcome types are not fully documented. Please:
- List all possible verification outcomes
- Document the meaning of each outcome
- Specify any outcome-specific fields or behaviors
This documentation is essential for API consumers to properly handle all possible response scenarios.
…nkey into rfc-0005-analytics-api
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
apps/engineering/content/rfcs/0005-analytics-api.mdx (5)
13-19: Consider enhancing the motivation section with concrete examplesWhile the motivation is clear, consider strengthening it with:
- Specific examples of API usage thresholds where direct Stripe event emission becomes problematic
- Quantitative metrics showing the performance impact of querying events from Stripe
- Common use cases that demonstrate why granular analytics are essential
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
31-40: Consider data privacy implications of exposed fieldsThe ClickHouse schema includes potentially sensitive fields like
workspace_id,key_id, andidentity_id. Please ensure:
- Appropriate field-level access controls are in place
- Data is properly anonymized or filtered based on the caller's permissions
- PII handling complies with relevant regulations (GDPR, CCPA, etc.)
44-60: Document performance implications of different granularitiesFor each granularity level (hourly, daily, monthly), consider documenting:
- Expected query performance characteristics
- Data retention periods
- Rate limiting considerations
- Recommended usage patterns for different data volumes
261-274: Expand implementation considerations with concrete solutionsConsider addressing these implementation concerns more thoroughly:
For serverless costs:
- Propose specific caching strategies (Redis, CDN, etc.)
- Consider implementing request batching
- Evaluate edge computing options
For key details fetching:
- Consider including minimal key metadata in the response
- Evaluate GraphQL as an alternative to reduce round trips
- Propose batch endpoints for fetching key details
Would you like me to help draft detailed technical specifications for these solutions?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~261-~261: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ustomer's users could generate a decent amount of requests. ## Alternatives Offering...(AMOUNTOF_TO_NUMBEROF)
[typographical] ~265-~265: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: .../metricsendpoint would be interesting, however I believe most of our users don't have ...(HOWEVER_SENTENCE)
[style] ~274-~274: Consider a shorter alternative to avoid wordiness.
Context: ...r needs to make another call to our API in order to fetch details such as the name for each...(IN_ORDER_TO_PREMIUM)
14-14: Fix grammar and style issuesSeveral grammatical improvements would enhance readability:
-Furthermore if you want to build end-user facing or internal analytics +Furthermore, if you want to build end-user facing or internal analytics -For example selecting `hour` +For example, selecting `hour` -However in certain scenarios +However, in certain scenarios -For example finding out +For example, finding out -interesting, however I believe +interesting; however, I believeAlso applies to: 93-93, 123-123, 124-124, 265-265
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/engineering/content/rfcs/0005-analytics-api.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/rfcs/0005-analytics-api.mdx
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~22-~22: Consider a shorter alternative to avoid wordiness.
Context: ...s data via an API. ## Detailed design In order to charge for usage, our users need inform...
(IN_ORDER_TO_PREMIUM)
[grammar] ~23-~23: The correct preposition here is “on” or “about”.
Context: ...e for usage, our users need information of who used their API when and **h...
(INFORMATION_OF)
[grammar] ~75-~75: The singular proper name ‘Unix’ must be used with a third-person or a past tense verb.
Context: ...` #### start (integer, required) Unix timestamp in milliseconds to specify the start of...
(HE_VERB_AGR)
[grammar] ~84-~84: The singular proper name ‘Unix’ must be used with a third-person or a past tense verb.
Context: ...mer #### end (integer, required) Unix timestamp in milliseconds to specify the end of t...
(HE_VERB_AGR)
[typographical] ~93-~93: After the expression ‘for example’ a comma is usually used.
Context: ...) Selects the granularity of data. For example selecting hour will return one datapo...
(COMMA_FOR_EXAMPLE)
[uncategorized] ~123-~123: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...rifications across identities and keys. However in certain scenarios you want to get a ...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[typographical] ~124-~124: After the expression ‘for example’ a comma is usually used.
Context: ...ou want to get a breakdown per key. For example finding out the usage spread across all...
(COMMA_FOR_EXAMPLE)
[typographical] ~132-~132: Two consecutive dots
Context: ... #### orderBy (enum ["total", "valid", ..], optional) <Callout type="warn" title...
(DOUBLE_PUNCTUATION)
[uncategorized] ~261-~261: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ustomer's users could generate a decent amount of requests. ## Alternatives Offering...
(AMOUNTOF_TO_NUMBEROF)
[typographical] ~265-~265: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: .../metrics endpoint would be interesting, however I believe most of our users don't have ...
(HOWEVER_SENTENCE)
[style] ~274-~274: Consider a shorter alternative to avoid wordiness.
Context: ...r needs to make another call to our API in order to fetch details such as the name for each...
(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (1)
apps/engineering/content/rfcs/0005-analytics-api.mdx (1)
65-66: Complete the permission requirements specification
The permission requirements for the root key are still marked as TBD. This is a blocker for implementation.
| type Datapoint = { | ||
| /** | ||
| * Unix timestamp in milliseconds of the start of the current time slice. | ||
| */ | ||
| time: number | ||
|
|
||
|
|
||
| /** | ||
| * For brevity, I will not explain every outcome here. | ||
| * There will be one key and count for every possible outcome, so you may | ||
| * choose what to display or not. | ||
| */ | ||
| valid: number | ||
| rateLimited: number | ||
| usageExceeded: number | ||
| // ... | ||
|
|
||
| /** | ||
| * Total number of verifications in the current time slice, regardless of outcome. | ||
| */ | ||
| total: number | ||
|
|
||
| /** | ||
| * Only available when specifying groupBy in the query. | ||
| * In this case there would be one datapoint per time and groupBy target. | ||
| */ | ||
| keyId?: string | ||
| apiId?: string | ||
| identity?: { | ||
| id: string | ||
| externalId: string | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type documentation with value constraints
The Datapoint type needs additional documentation:
- Valid ranges for numeric fields
- Format specifications for ID fields
- Enum values for possible outcomes
- Maximum length constraints for string fields
Consider adding TypeScript type definitions for the outcome values:
type VerificationOutcome =
| "valid"
| "rateLimited"
| "usageExceeded"
| "insufficientPermissions"
// Add other possible outcomes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/engineering/content/rfcs/0005-analytics-api.mdx (3)
13-19: Consider enhancing the motivation section with concrete examplesWhile the motivation is clear, consider adding:
- Specific examples of API usage patterns that make direct Stripe integration impractical
- Quantitative thresholds for when direct event emission becomes "noisy"
- Common use cases that require finer granularity than what Stripe provides
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
70-70: Fix grammatical error in parameter documentationThe sentence contains a typo and is difficult to parse. Consider rewording:
- All required and optional arguments are passed via query parameters. Some paramters may be specific multiple times, either as You may specify multiple ids such as `?param=value_1,value_2` or `?param=value_1¶m=value_2` + All required and optional arguments are passed via query parameters. Some parameters may be specified multiple times using either comma-separated values (`?param=value_1,value_2`) or multiple parameters (`?param=value_1¶m=value_2`)
261-275: Expand on performance implications and mitigation strategiesThe drawbacks and unresolved questions sections would benefit from more detailed discussion of:
- Expected query patterns and their impact on costs
- Potential caching strategies and their trade-offs
- Specific retention quotas for different tiers
- Performance implications of groupBy queries and potential optimizations
Would you like assistance in drafting a more detailed analysis of these aspects?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~261-~261: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ustomer's users could generate a decent amount of requests. ## Alternatives Offering...(AMOUNTOF_TO_NUMBEROF)
[typographical] ~265-~265: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: .../metricsendpoint would be interesting, however I believe most of our users don't have ...(HOWEVER_SENTENCE)
[style] ~274-~274: Consider a shorter alternative to avoid wordiness.
Context: ...r needs to make another call to our API in order to fetch details such as the name for each...(IN_ORDER_TO_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/engineering/content/rfcs/0005-analytics-api.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/rfcs/0005-analytics-api.mdx
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~22-~22: Consider a shorter alternative to avoid wordiness.
Context: ...s data via an API. ## Detailed design In order to charge for usage, our users need inform...
(IN_ORDER_TO_PREMIUM)
[grammar] ~23-~23: The correct preposition here is “on” or “about”.
Context: ...e for usage, our users need information of who used their API when and **h...
(INFORMATION_OF)
[uncategorized] ~50-~50: Possible missing preposition found.
Context: ...: - hourly - daily - monthly In order be scalable, we will not expose individual...
(AI_HYDRA_LEO_MISSING_TO)
[grammar] ~75-~75: The singular proper name ‘Unix’ must be used with a third-person or a past tense verb.
Context: ...` #### start (integer, required) Unix timestamp in milliseconds to specify the start of...
(HE_VERB_AGR)
[grammar] ~84-~84: The singular proper name ‘Unix’ must be used with a third-person or a past tense verb.
Context: ...mer #### end (integer, required) Unix timestamp in milliseconds to specify the end of t...
(HE_VERB_AGR)
[typographical] ~93-~93: After the expression ‘for example’ a comma is usually used.
Context: ...) Selects the granularity of data. For example selecting hour will return one datapo...
(COMMA_FOR_EXAMPLE)
[uncategorized] ~123-~123: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...rifications across identities and keys. However in certain scenarios you want to get a ...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[typographical] ~124-~124: After the expression ‘for example’ a comma is usually used.
Context: ...ou want to get a breakdown per key. For example finding out the usage spread across all...
(COMMA_FOR_EXAMPLE)
[typographical] ~132-~132: Two consecutive dots
Context: ... #### orderBy (enum ["total", "valid", ..], optional) <Callout type="warn" title...
(DOUBLE_PUNCTUATION)
[uncategorized] ~261-~261: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ustomer's users could generate a decent amount of requests. ## Alternatives Offering...
(AMOUNTOF_TO_NUMBEROF)
[typographical] ~265-~265: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: .../metrics endpoint would be interesting, however I believe most of our users don't have ...
(HOWEVER_SENTENCE)
[style] ~274-~274: Consider a shorter alternative to avoid wordiness.
Context: ...r needs to make another call to our API in order to fetch details such as the name for each...
(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (2)
apps/engineering/content/rfcs/0005-analytics-api.mdx (2)
65-66: Complete the permission requirements specification
The permission requirements for the root key are still marked as TBD. This should be defined before implementation to ensure proper access control.
223-256: 🛠️ Refactor suggestion
Enhance type documentation with validation constraints
The Datapoint type definition needs additional details:
- Valid ranges for numeric fields (e.g., are negative counts allowed?)
- Format specifications for ID fields
- Complete list of possible outcomes
- Maximum length constraints for string fields
Consider adding JSDoc comments for validation constraints:
type Datapoint = {
/**
* Unix timestamp in milliseconds of the start of the current time slice.
+ * @minimum 0
*/
time: number
/**
* For brevity, I will not explain every outcome here.
* There will be one key and count for every possible outcome, so you may
* choose what to display or not.
+ * @minimum 0
*/
valid: number
rateLimited: number
usageExceeded: number
// ...
/**
* Total number of verifications in the current time slice, regardless of outcome.
+ * @minimum 0
*/
total: number
/**
* Only available when specifying groupBy in the query.
* In this case there would be one datapoint per time and groupBy target.
+ * @pattern ^[a-zA-Z0-9_-]+$
*/
keyId?: string
apiId?: string
identity?: {
id: string
externalId: string
}
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apps/engineering/content/rfcs/0005-analytics-api.mdx (3)
13-19: Enhance motivation with specific examples and quantitative benefitsConsider strengthening the motivation section by:
- Adding specific examples of granularity limitations in Stripe
- Including quantitative benefits of using Unkey's solution (e.g., data retention period, query performance)
- Describing typical use cases that are difficult to implement with existing solutions
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
131-140: Define concrete orderBy functionalityThe orderBy functionality is marked as WIP. Consider:
- Finalizing the list of sortable fields
- Documenting performance implications
- Adding examples for each sorting option
- Specifying default sorting behavior
Would you like help designing the complete orderBy API specification?
🧰 Tools
🪛 LanguageTool
[typographical] ~132-~132: Two consecutive dots
Context: ... #### orderBy (enum ["total", "valid", ..], optional) <Callout type="warn" title...(DOUBLE_PUNCTUATION)
261-261: Elaborate on cost mitigation strategiesThe serverless cost concern needs more detail:
- Estimated cost per request
- Proposed caching strategy
- Rate limiting policies
- Cost optimization techniques
🧰 Tools
🪛 LanguageTool
[uncategorized] ~261-~261: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ustomer's users could generate a decent amount of requests. ## Alternatives Offering...(AMOUNTOF_TO_NUMBEROF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/engineering/content/rfcs/0005-analytics-api.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/rfcs/0005-analytics-api.mdx
[uncategorized] ~14-~14: A comma may be missing after the conjunctive/linking adverb ‘Furthermore’.
Context: ...r, but this becomes very noisy quickly. Furthermore if you want to build end-user facing or...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~22-~22: Consider a shorter alternative to avoid wordiness.
Context: ...s data via an API. ## Detailed design In order to charge for usage, our users need inform...
(IN_ORDER_TO_PREMIUM)
[grammar] ~23-~23: The correct preposition here is “on” or “about”.
Context: ...e for usage, our users need information of who used their API when and **h...
(INFORMATION_OF)
[grammar] ~75-~75: The singular proper name ‘Unix’ must be used with a third-person or a past tense verb.
Context: ...` #### start (integer, required) Unix timestamp in milliseconds to specify the start of...
(HE_VERB_AGR)
[grammar] ~84-~84: The singular proper name ‘Unix’ must be used with a third-person or a past tense verb.
Context: ...mer #### end (integer, required) Unix timestamp in milliseconds to specify the end of t...
(HE_VERB_AGR)
[typographical] ~93-~93: After the expression ‘for example’ a comma is usually used.
Context: ...) Selects the granularity of data. For example selecting hour will return one datapo...
(COMMA_FOR_EXAMPLE)
[uncategorized] ~123-~123: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...rifications across identities and keys. However in certain scenarios you want to get a ...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[typographical] ~124-~124: After the expression ‘for example’ a comma is usually used.
Context: ...ou want to get a breakdown per key. For example finding out the usage spread across all...
(COMMA_FOR_EXAMPLE)
[typographical] ~132-~132: Two consecutive dots
Context: ... #### orderBy (enum ["total", "valid", ..], optional) <Callout type="warn" title...
(DOUBLE_PUNCTUATION)
[uncategorized] ~261-~261: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ustomer's users could generate a decent amount of requests. ## Alternatives Offering...
(AMOUNTOF_TO_NUMBEROF)
[typographical] ~265-~265: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: .../metrics endpoint would be interesting, however I believe most of our users don't have ...
(HOWEVER_SENTENCE)
[style] ~274-~274: Consider a shorter alternative to avoid wordiness.
Context: ...r needs to make another call to our API in order to fetch details such as the name for each...
(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (2)
apps/engineering/content/rfcs/0005-analytics-api.mdx (2)
65-66: Permission requirements still undefined
The permission requirements for the root key are still marked as TBD.
273-275: Address key implementation challenges
Critical implementation details need to be resolved:
- Define acceptable cache times based on data freshness requirements
- Consider implementing a batch API for fetching key details
- Document retention quotas per tier
- Specify granularity limitations per tier
Let's verify the current key details API performance:
🧰 Tools
🪛 LanguageTool
[style] ~274-~274: Consider a shorter alternative to avoid wordiness.
Context: ...r needs to make another call to our API in order to fetch details such as the name for each...
(IN_ORDER_TO_PREMIUM)
| ```sql | ||
| `request_id` String, | ||
| `time` Int64, | ||
| `workspace_id` String, | ||
| `key_space_id` String, | ||
| `key_id` String, | ||
| `region` LowCardinality(String), | ||
| `outcome` LowCardinality(String), | ||
| `identity_id` String | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document field constraints and data types
The ClickHouse schema needs additional documentation:
- Maximum length for String fields
- Format requirements for IDs
- Possible values for
outcome - Time range limitations for
time - Region format/possible values
| All required and optional arguments are passed via query parameters. Some parameters may be specified multiple times, either as You may specify multiple ids such as `?param=value_1,value_2` or `?param=value_1¶m=value_2` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify parameter validation rules
The documentation should specify:
- Maximum number of IDs allowed in multi-value parameters
- Maximum length for ID parameters
- Rate limiting considerations for large queries
- Maximum time range allowed per granularity level
| Successful responses will always return an array of datapoints. One datapoint per granular slice, ie: hourly granularity means you receive one element per hour within the queried interval. | ||
| ```json title="200 OK Body" | ||
| [ | ||
| Datapoint, | ||
| Datapoint, | ||
| Datapoint | ||
| ] | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error responses and rate limiting documentation
The response documentation should include:
- Error response formats
- Rate limiting headers
- Pagination metadata (if applicable)
- Cache control headers
Summary by CodeRabbit
Release Notes
New Features
Improvements
Handshaketab.Bug Fixes