Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug logs for RedisCache behaviour #8355

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Debug logs for RedisCache behaviour #8355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11e06a7
debug logs
enisdenjo Feb 3, 2025
071b199
changeset
enisdenjo Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/many-boxes-nail.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@graphql-mesh/cache-redis': patch
---

Debug logs for caching behaviour
29 changes: 24 additions & 5 deletions packages/cache/redis/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ function interpolateStrWithEnv(str: string): string {

export default class RedisCache<V = string> implements KeyValueCache<V>, Disposable {
private client: Redis;
private logger: Logger;

constructor(options: YamlConfig.Cache['redis'] & { pubsub?: MeshPubSub; logger: Logger }) {
this.logger = options.logger;
const lazyConnect = options.lazyConnect !== false;
if (options.url) {
const redisUrl = new URL(interpolateStrWithEnv(options.url));
Expand All @@ -40,7 +42,7 @@ export default class RedisCache<V = string> implements KeyValueCache<V>, Disposa
redisUrl.searchParams.set('family', '6');
}
const urlStr = redisUrl.toString();
options.logger.debug(`Connecting to Redis at ${urlStr}`);
this.logger.debug(`Connecting to Redis at ${urlStr}`);
this.client = new Redis(urlStr);
} else {
const parsedHost = interpolateStrWithEnv(options.host?.toString()) || process.env.REDIS_HOST;
Expand All @@ -53,7 +55,7 @@ export default class RedisCache<V = string> implements KeyValueCache<V>, Disposa
const numPort = parseInt(parsedPort);
const numDb = parseInt(parsedDb);
if (parsedHost) {
options.logger.debug(`Connecting to Redis at ${parsedHost}:${parsedPort}`);
this.logger.debug(`Connecting to Redis at ${parsedHost}:${parsedPort}`);
this.client = new Redis({
host: parsedHost,
port: isNaN(numPort) ? undefined : numPort,
Expand All @@ -65,7 +67,7 @@ export default class RedisCache<V = string> implements KeyValueCache<V>, Disposa
enableOfflineQueue: true,
});
} else {
options.logger.debug(`Connecting to Redis mock`);
this.logger.debug(`Connecting to Redis mock`);
this.client = new RedisMock();
}
}
Expand All @@ -83,15 +85,27 @@ export default class RedisCache<V = string> implements KeyValueCache<V>, Disposa
set(key: string, value: V, options?: KeyValueCacheSetOptions): Promise<any> {
const stringifiedValue = JSON.stringify(value);
if (options?.ttl && options.ttl > 0) {
this.logger.debug(
`Caching using key "${key}" with ${options.ttl * 1000}s TTL`,
stringifiedValue,
);
return this.client.set(key, stringifiedValue, 'PX', options.ttl * 1000);
} else {
this.logger.debug(`Caching using key "${key}"`, stringifiedValue);
return this.client.set(key, stringifiedValue);
}
}

get(key: string): Promise<V | undefined> {
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;
Comment on lines +100 to +108
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize value logging and handle potential JSON.parse errors.

The current implementation has two areas for improvement:

  1. Similar to set operation, logging full values could expose sensitive data
  2. 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.

Suggested change
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;
}
});

});
}

Expand All @@ -101,12 +115,17 @@ export default class RedisCache<V = string> implements KeyValueCache<V>, Disposa

delete(key: string): PromiseLike<boolean> | boolean {
try {
this.logger.debug(`Deleting "${key}" from cache`);
return mapMaybePromise(
this.client.del(key),
value => value > 0,
() => false,
e => {
this.logger.error(`Error deleting "${key}" from cache`, e);
return false;
},
);
} catch (e) {
this.logger.error(`Error trying to delete "${key}" from cache`, e);
return false;
}
}
Expand Down
Loading