chore: more logging and retries #2475
Conversation
chronark
requested review from
perkinsjr,
mcstepp and
MichaelUnkey
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KeyService
participant Database
Client->>KeyService: verifyKey(key)
KeyService->>KeyService: Attempt verification
alt Verification fails
KeyService->>KeyService: Log warning
KeyService->>KeyService: Retry verification
KeyService->>Database: _verifyKey(query)
Database-->>KeyService: Return result
end
KeyService-->>Client: Return verification result
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 (
|
|
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: 2
🧹 Outside diff range and nitpick comments (1)
internal/metrics/src/index.ts (1)
73-74: Approve changes with security and performance considerations.The addition of
dbResandsqloptional properties to the "metric.db.read" object enhances the schema's capability for detailed database operation logging. This can significantly improve debugging and monitoring capabilities.However, please consider the following:
- Security: Ensure that the
sqlproperty doesn't log sensitive information (e.g., PII, passwords) that could pose a security risk if exposed in logs.- Performance: The
dbResproperty might store large amounts of data. Consider implementing size limits or truncation to prevent excessive storage or processing overhead.- Data Retention: Review your data retention policies to ensure compliance with relevant data protection regulations, especially if these new properties might contain sensitive information.
Consider implementing the following safeguards:
- Add a sanitization step for the
sqlproperty to remove or mask sensitive data before logging.- Implement size limits for the
dbResproperty to prevent performance issues.- Ensure your logging system has appropriate access controls and encryption for these potentially sensitive fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/api/src/pkg/keys/service.ts (3 hunks)
- internal/metrics/src/index.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
apps/api/src/pkg/keys/service.ts (1)
Line range hint
224-273: Approved: Refactoring Enhances Readability and LoggingAssigning the database query to a variable
querybefore execution improves the readability of the code and allows for enhanced logging of the SQL query. This change facilitates better traceability of database interactions.
| let res = await this._verifyKey(c, req); | ||
| let remainingRetries = 3; | ||
| while (res.err && remainingRetries > 0) { | ||
| remainingRetries--; | ||
| this.logger.warn("retrying verification", { | ||
| remainingRetries, | ||
| previousError: res.err.message, | ||
| }); | ||
|
|
||
| await this.cache.keyByHash.remove(await this.hash(req.key)); | ||
| res = await this._verifyKey(c, req); | ||
| } | ||
|
|
There was a problem hiding this comment.
Redundant Retry Mechanisms in verifyKey and _verifyKey
The verifyKey method introduces a retry loop that attempts to call _verifyKey up to three times if an error occurs. However, the _verifyKey method already includes its own retry logic when fetching key data from the cache and database using the retry function.
This redundancy could lead to unnecessary duplication of retries, increasing the overall number of attempts and potentially prolonging response times. Consider consolidating the retry logic to a single location to improve efficiency. You might handle retries either in verifyKey or within _verifyKey, but not both.
| dbRes: JSON.stringify(dbRes), | ||
| sql: query.toSQL().sql, |
There was a problem hiding this comment.
Potential Sensitive Data Exposure in Logs
Logging the entire database response (dbRes) and the raw SQL query (sql) may expose sensitive information, including personal data or security-related details. This could pose a security risk if logs are accessed by unauthorized parties.
Consider sanitizing or limiting the data being logged to exclude sensitive information. Instead of logging the full dbRes, log only non-sensitive attributes necessary for debugging. Similarly, ensure that the logged SQL query does not contain sensitive parameters. Here's a suggested change:
this.metrics.emit({
metric: "metric.db.read",
query: "getKeyAndApiByHash",
latency: performance.now() - dbStart,
- dbRes: JSON.stringify(dbRes),
- sql: query.toSQL().sql,
+ resultSize: dbRes ? 1 : 0,
+ queryInfo: { /* include non-sensitive query details if necessary */ },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dbRes: JSON.stringify(dbRes), | |
| sql: query.toSQL().sql, | |
| resultSize: dbRes ? 1 : 0, | |
| queryInfo: { /* include non-sensitive query details if necessary */ }, |
* fix: Missing plan check and ip whitelist parsing * fix: adjust tests for ipwhitelist * fix: Rename error code Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: ipwhitelist via features vs enterprise plan * fix: invert condition * chore: add cache log * fix: ensure workspace is loaded (#2470) * chore: more logging and retries (#2475) * Update 7_create_a_template.md (#2471) * increase override limit * Update create-new-override.tsx * fix: Enhance API Key Detail Page: Change Permissions Visualization to Tree Format (#2238) * fix:changed the permission view * fixed issue comments * [autofix.ci] apply automated fixes * removed font --------- Co-authored-by: Andreas Thomas <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * fix: show shallow permissions followed by nested in alphabetical order (#2273) * fix: show shallow permissions followed by nested in alphabetical order * fix: correct the sorting of nested permissions top level keys * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * Add Template to Markdown (#2362) Co-authored-by: Andreas Thomas <[email protected]> * fix: retry on any error with disabled cache --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: chronark <[email protected]> Co-authored-by: Chirag Arora <[email protected]> Co-authored-by: RajuGangitla <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Anne Deepa Prasanna <[email protected]> Co-authored-by: djnovin <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation