Debug logs for RedisCache behaviour #8355
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/cache/redis/src/index.tsOops! Something went wrong! :( ESLint: 9.19.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📝 WalkthroughWalkthroughThis pull request enhances debug logging in the caching package that interfaces with Redis. The changes introduce a dedicated logger within the Redis cache class, logging Redis connection attempts and caching operations (set, get, and delete). The improved logging adds visibility into cache operations, aiding in troubleshooting and performance monitoring, without altering any public interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Application
participant RC as RedisCache
participant Log as Logger
participant Redis as Redis Server
Client->>RC: Instantiate RedisCache(options)
RC->>Log: Log Redis connection attempt
RC->>Redis: Initiate connection
Redis-->>RC: Connection response
sequenceDiagram
participant Client as Application
participant RC as RedisCache
participant Log as Logger
participant Redis as Redis Server
Client->>RC: set(key, value, ttl)
RC->>Log: Log set operation details (key, ttl)
RC->>Redis: Execute SET command
Redis-->>RC: Acknowledge storage
RC->>Log: Log operation result
RC-->>Client: Return set status
Poem
✨ Finishing Touches
🪧 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 (
|
Apollo Federation Subgraph Compatibility Results
Learn more: |
![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
🧹 Nitpick comments (3)
packages/cache/redis/src/index.ts (3)
45-45: Consider masking sensitive information in connection logs.While the connection logging is helpful for debugging, consider masking sensitive parts of the connection URL/details (like credentials) to prevent security risks if logs are exposed.
Apply this diff to mask sensitive information:
- this.logger.debug(`Connecting to Redis at ${urlStr}`); + const maskedUrl = urlStr.replace(/\/\/([^:]+):([^@]+)@/, '//***:***@'); + this.logger.debug(`Connecting to Redis at ${maskedUrl}`);- this.logger.debug(`Connecting to Redis at ${parsedHost}:${parsedPort}`); + this.logger.debug(`Connecting to Redis at ${parsedHost}:${parsedPort}${parsedUsername ? ' with credentials' : ''}`);Also applies to: 58-58, 70-70
88-91: Consider truncating and sanitizing cached values in logs.Including full values in debug logs could:
- Expose sensitive data
- Generate excessive log volume with large values
Apply this diff to truncate and sanitize values:
- this.logger.debug( - `Caching using key "${key}" with ${options.ttl * 1000}s TTL`, - stringifiedValue, - ); + const truncatedValue = stringifiedValue.length > 100 + ? `${stringifiedValue.substring(0, 100)}...` + : stringifiedValue; + this.logger.debug( + `Caching using key "${key}" with ${options.ttl * 1000}s TTL`, + truncatedValue, + );- this.logger.debug(`Caching using key "${key}"`, stringifiedValue); + const truncatedValue = stringifiedValue.length > 100 + ? `${stringifiedValue.substring(0, 100)}...` + : stringifiedValue; + this.logger.debug(`Caching using key "${key}"`, truncatedValue);Also applies to: 94-94
118-118: Consider consolidating error handling logic.The method has two separate error handling blocks with similar logging logic. Consider consolidating them for better maintainability.
Apply this diff to consolidate error handling:
delete(key: string): PromiseLike<boolean> | boolean { + const handleError = (e: Error, context: string) => { + this.logger.error(`Error ${context} "${key}" from cache`, e); + return false; + }; + try { this.logger.debug(`Deleting "${key}" from cache`); return mapMaybePromise( this.client.del(key), value => value > 0, - e => { - this.logger.error(`Error deleting "${key}" from cache`, e); - return false; - }, + e => handleError(e, 'deleting'), ); } catch (e) { - this.logger.error(`Error trying to delete "${key}" from cache`, e); - return false; + return handleError(e, 'trying to delete'); } }Also applies to: 123-124, 128-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/many-boxes-nail.md(1 hunks)packages/cache/redis/src/index.ts(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/many-boxes-nail.md
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: integration / node 22
- GitHub Check: unit / node 22
- GitHub Check: integration / node 20
- GitHub Check: e2e / node v22
- GitHub Check: unit / node 20
- GitHub Check: integration / node 18
- GitHub Check: e2e / node v20
- GitHub Check: unit / node 18
- GitHub Check: release / snapshot
- GitHub Check: e2e / node v18
- GitHub Check: check
- GitHub Check: deployment
🔇 Additional comments (1)
packages/cache/redis/src/index.ts (1)
21-24: LGTM! Well-structured logger initialization.Good practice to store the logger as a private class member, ensuring consistent access throughout the class methods.
| this.logger.debug(`Getting "${key}" from cache`); | ||
| return mapMaybePromise(this.client.get(key), value => { | ||
| return value != null ? JSON.parse(value) : undefined; | ||
| const val = value != null ? JSON.parse(value) : undefined; | ||
| if (val) { | ||
| this.logger.debug(`Cache hit using "${key}"`, val); | ||
| } else { | ||
| this.logger.debug(`Cache miss using "${key}"`); | ||
| } | ||
| return val; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize value logging and handle potential JSON.parse errors.
The current implementation has two areas for improvement:
- Similar to set operation, logging full values could expose sensitive data
- JSON.parse could throw for invalid JSON strings
Apply this diff to improve the implementation:
this.logger.debug(`Getting "${key}" from cache`);
return mapMaybePromise(this.client.get(key), value => {
- const val = value != null ? JSON.parse(value) : undefined;
- if (val) {
- this.logger.debug(`Cache hit using "${key}"`, val);
- } else {
- this.logger.debug(`Cache miss using "${key}"`);
- }
- return val;
+ try {
+ const val = value != null ? JSON.parse(value) : undefined;
+ if (val) {
+ const truncatedValue = JSON.stringify(val).length > 100
+ ? `${JSON.stringify(val).substring(0, 100)}...`
+ : JSON.stringify(val);
+ this.logger.debug(`Cache hit using "${key}"`, truncatedValue);
+ } else {
+ this.logger.debug(`Cache miss using "${key}"`);
+ }
+ return val;
+ } catch (e) {
+ this.logger.error(`Error parsing cached value for "${key}"`, e);
+ return undefined;
+ }📝 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.
| this.logger.debug(`Getting "${key}" from cache`); | |
| return mapMaybePromise(this.client.get(key), value => { | |
| return value != null ? JSON.parse(value) : undefined; | |
| const val = value != null ? JSON.parse(value) : undefined; | |
| if (val) { | |
| this.logger.debug(`Cache hit using "${key}"`, val); | |
| } else { | |
| this.logger.debug(`Cache miss using "${key}"`); | |
| } | |
| return val; | |
| this.logger.debug(`Getting "${key}" from cache`); | |
| return mapMaybePromise(this.client.get(key), value => { | |
| try { | |
| const val = value != null ? JSON.parse(value) : undefined; | |
| if (val) { | |
| const truncatedValue = JSON.stringify(val).length > 100 | |
| ? `${JSON.stringify(val).substring(0, 100)}...` | |
| : JSON.stringify(val); | |
| this.logger.debug(`Cache hit using "${key}"`, truncatedValue); | |
| } else { | |
| this.logger.debug(`Cache miss using "${key}"`); | |
| } | |
| return val; | |
| } catch (e) { | |
| this.logger.error(`Error parsing cached value for "${key}"`, e); | |
| return undefined; | |
| } | |
| }); |
💻 Website PreviewThe latest changes are available as preview in: https://f966323a.graphql-mesh.pages.dev |
Summary by CodeRabbit