feat(cache-redis): add OTEL traces #8830
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds OpenTelemetry tracing to @graphql-mesh/cache-redis (init, get, set, delete) and introduces a runtime dependency on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Cache as RedisCache
participant OTEL as OpenTelemetry Tracer
participant Redis as Redis Server/Cluster
rect rgba(200,230,255,0.25)
note over Cache,OTEL: Initialization
App->>Cache: new RedisCache(options)
Cache->>OTEL: startActiveSpan("hive.cache.redis.init")
Cache->>Redis: connect (url/cluster/sentinel/mock)
Redis-->>Cache: ready/connected or error
Cache->>OTEL: end span
end
rect rgba(220,255,220,0.15)
note over App,Redis: Get
App->>Cache: get(key)
Cache->>OTEL: startActiveSpan("hive.cache.get")
Cache->>Redis: GET key
Redis-->>Cache: value/null or error
Cache->>OTEL: end span
Cache-->>App: value/null or error
end
rect rgba(255,240,200,0.15)
note over App,Redis: Set
App->>Cache: set(key, value, ttl)
Cache->>OTEL: startActiveSpan("hive.cache.set")
Cache->>Redis: SET key value EX ttl
Redis-->>Cache: OK or error
Cache->>OTEL: end span
Cache-->>App: result or error
end
rect rgba(255,220,220,0.15)
note over App,Redis: Delete
App->>Cache: delete(key)
Cache->>OTEL: startActiveSpan("hive.cache.delete")
Cache->>Redis: DEL key
Redis-->>Cache: count or error
Cache->>OTEL: end span
Cache-->>App: count or error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
packages/cache/redis/src/index.ts (1)
165-167: Consider adding tracing togetKeysByPrefix.The
getKeysByPrefixmethod doesn't have OpenTelemetry tracing while other cache operations (get,set,delete) do. Adding a span here would provide more complete observability coverage.You could add tracing similar to the other methods:
getKeysByPrefix(prefix: string): Promise<string[]> { - return scanPatterns(this.client, `${prefix}*`); + return this.tracer.startActiveSpan('hive.cache.getKeysByPrefix', span => + scanPatterns(this.client, `${prefix}*`).finally(() => span.end()), + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.changeset/@graphql-mesh_cache-redis-8830-dependencies.md(1 hunks).changeset/shiny-paths-flow.md(1 hunks)packages/cache/redis/package.json(1 hunks)packages/cache/redis/src/index.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/cache/redis/src/index.ts (3)
packages/legacy/types/src/index.ts (1)
Logger(202-211)packages/legacy/types/src/config.ts (1)
Cache(1808-1817)packages/legacy/types/src/pubsub.ts (3)
MeshPubSub(14-24)HivePubSub(6-6)toMeshPubSub(164-175)
🔇 Additional comments (7)
.changeset/shiny-paths-flow.md (1)
1-5: LGTM!The changeset correctly documents the minor version bump and the new OTEL tracing feature for initialization and cache operations.
packages/cache/redis/src/index.ts (4)
14-14: LGTM!The OpenTelemetry imports are correct and align with the newly added dependency.
23-23: LGTM!The tracer field declaration is appropriate for storing the OpenTelemetry tracer instance.
144-155: LGTM!The
setmethod correctly wraps the Redis operation in a span and ensures proper span termination usingfinallyblocks in both the TTL and non-TTL paths.
169-179: LGTM!The
deletemethod correctly wraps the Redis operation in a span and ensures the span is ended using afinallyblock..changeset/@graphql-mesh_cache-redis-8830-dependencies.md (1)
1-5: LGTM!The changeset correctly documents the patch version bump and the addition of the
@opentelemetry/apidependency.packages/cache/redis/package.json (1)
41-41: Verified: @opentelemetry/api@^1.9.0 is current and secure
Latest release is 1.9.0 and no known vulnerabilities were found.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cache/redis/src/index.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/cache/redis/src/index.ts (3)
packages/legacy/types/src/index.ts (1)
Logger(202-211)packages/legacy/types/src/config.ts (1)
Cache(1808-1817)packages/legacy/types/src/pubsub.ts (3)
MeshPubSub(14-24)HivePubSub(6-6)toMeshPubSub(164-175)
🔇 Additional comments (4)
packages/cache/redis/src/index.ts (4)
14-14: LGTM!The OpenTelemetry imports and tracer field are correctly added.
Also applies to: 23-23
28-140: LGTM! Past review comment addressed.The try-finally pattern ensures the span ends even if initialization fails, addressing the previous concern about span leaks.
160-167: LGTM! Past review comment addressed.The promise chain is now correct, with
.finally(() => span.end())properly chained to the promise. The span will end in all code paths.
173-182: LGTM!The delete method correctly traces the operation with the span ending in all code paths via
.finally().
EmrysMyrddin
force-pushed
the
feat/cache-redis/add-otel-traces
branch
from
40e32e9 to
4eeae94
Compare
Apollo Federation Subgraph Compatibility Results
Learn more: |
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-mesh/cache-redis |
0.105.0-alpha-20251010093057-4eeae9412ec39cfbab6e001ecaf527cf94f4ea31 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/apollo-link |
0.106.14-alpha-20251010093057-4eeae9412ec39cfbab6e001ecaf527cf94f4ea31 |
npm ↗︎ unpkg ↗︎ |
💻 Website PreviewThe latest changes are available as preview in: https://b1d9fdf3.graphql-mesh.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/cache/redis/package.json (1)
41-41: Re-evaluate dependency type for @opentelemetry/apiTo avoid duplicate API singletons across the monorepo/apps, consider making @opentelemetry/api a peerDependency (and keep as a regular dependency only if you explicitly want to bundle it here). Many OTEL libraries use a peer dependency on ^1.0.0 to share the global API.
packages/cache/redis/src/index.ts (5)
85-91: Prefer passing ioredis options programmatically over URL query paramsioredis does not support enableAutoPipelining/enableOfflineQueue/lazyConnect via connection string query params. Pass them via the options argument to new Redis(url, options) to ensure they take effect.
- if (lazyConnect) { - redisUrl.searchParams.set('lazyConnect', 'true'); - } - - redisUrl.searchParams.set('enableAutoPipelining', 'true'); - redisUrl.searchParams.set('enableOfflineQueue', 'true'); - const IPV6_REGEX = + const IPV6_REGEX = /^(?:(?:[a-fA-F\d]{1,4}:){7}(?:[a-fA-F\d]{1,4}|:)|(?:[a-fA-F\d]{1,4}:){6}(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|:[a-fA-F\d]{1,4}|:)|(?:[a-fA-F\d]{1,4}:){5}(?::(?:25[0-5]|2[0-5]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,2}|:)|(?:[a-fA-F\d]{1,4}:){4}(?:(?::[a-fA-F\d]{1,4}){0,1}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,3}|:)|(?:[a-fA-F\d]{1,4}:){3}(?:(?::[a-fA-F\d]{1,4}){0,2}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,4}|:)|(?:[a-fA-F\d]{1,4}:){2}(?:(?::[a-fA-F\d]{1,4}){0,3}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,5}|:)|(?:[a-fA-F\d]{1,4}:){1}(?:(?::[a-fA-F\d]{1,4}){0,4}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,6}|:)|(?::(?:(?::[a-fA-F\d]{1,4}){0,5}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,7}|:)))(?:%[0-9a-zA-Z]{1,})?$/gm; - if (IPV6_REGEX.test(redisUrl.hostname)) { - redisUrl.searchParams.set('family', '6'); - } - const urlStr = redisUrl.toString(); - options.logger.debug(`Connecting to Redis at ${urlStr}`); - this.client = new Redis(urlStr); + const family = IPV6_REGEX.test(redisUrl.hostname) ? 6 : undefined; + const urlStr = redisUrl.toString(); + const logUrl = new URL(urlStr); + if (logUrl.password) { + logUrl.password = '***'; + } + options.logger.debug(`Connecting to Redis at ${logUrl.toString()}`); + this.client = new Redis(urlStr, { + enableAutoPipelining: true, + enableOfflineQueue: true, + ...(lazyConnect ? { lazyConnect: true } : {}), + ...(family ? { family } : {}), + });Also applies to: 96-99
38-44: Add radix to parseInt for clarity and correctnessExplicitly pass 10 to parseInt to avoid edge cases and improve readability.
- const numDb = parseInt(parsedDb); + const numDb = parseInt(parsedDb, 10); ... - port: s.port && parseInt(interpolateStrWithEnv(s.port)), - family: s.family && parseInt(interpolateStrWithEnv(s.family)), + port: s.port && parseInt(interpolateStrWithEnv(s.port), 10), + family: s.family && parseInt(interpolateStrWithEnv(s.family), 10), ... - port: s.port && parseInt(interpolateStrWithEnv(s.port)), - family: s.family && parseInt(interpolateStrWithEnv(s.family)), + port: s.port && parseInt(interpolateStrWithEnv(s.port), 10), + family: s.family && parseInt(interpolateStrWithEnv(s.family), 10), ... - const numPort = parseInt(parsedPort); - const numDb = parseInt(parsedDb); + const numPort = parseInt(parsedPort, 10); + const numDb = parseInt(parsedDb, 10);Also applies to: 69-71, 112-113
131-136: Guard unsubscribe call against undefined valuesid and pubsub can be undefined at type-level. Guard to satisfy strict TS and avoid accidental runtime misuse.
- const id = pubsub?.subscribe('destroy', () => { - this.client.disconnect(false); - pubsub.unsubscribe(id); - }); + const id = pubsub?.subscribe('destroy', () => { + this.client.disconnect(false); + if (pubsub && id != null) { + pubsub.unsubscribe(id); + } + });
14-14: Record exceptions and set span status on errorsImprove observability by recording exceptions and setting SpanStatusCode.ERROR when operations fail.
-import { trace, type Tracer } from '@opentelemetry/api'; +import { trace, type Tracer, SpanStatusCode } from '@opentelemetry/api'; @@ set(key: string, value: V, options?: KeyValueCacheSetOptions): Promise<any> { return this.tracer.startActiveSpan('hive.cache.set', async span => { - try { + try { const stringifiedValue = JSON.stringify(value); if (options?.ttl && options.ttl > 0) { return await this.client.set(key, stringifiedValue, 'PX', options.ttl * 1000); } else { return await this.client.set(key, stringifiedValue); } - } finally { + } catch (err) { + span.recordException(err as Error); + span.setStatus({ code: SpanStatusCode.ERROR }); + throw err; + } finally { span.end(); } }); } @@ get(key: string): Promise<V | undefined> { return this.tracer.startActiveSpan('hive.cache.get', span => this.client .get(key) .then(value => (value != null ? JSON.parse(value) : undefined)) + .catch(err => { + span.recordException(err as Error); + span.setStatus({ code: SpanStatusCode.ERROR }); + throw err; + }) .finally(() => span.end()), ); } @@ delete(key: string): Promise<boolean> { return this.tracer.startActiveSpan('hive.cache.delete', span => this.client .del(key) .then( value => value > 0, () => false, ) + .catch(err => { + span.recordException(err as Error); + span.setStatus({ code: SpanStatusCode.ERROR }); + throw err; + }) .finally(() => span.end()), ); }Also applies to: 147-159, 162-169, 175-184
28-31: Optional: align tracer/span naming with packageConsider using '@graphql-mesh/cache-redis' as tracer name and spans like 'cache.redis.init|get|set|delete' for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.changeset/@graphql-mesh_cache-redis-8830-dependencies.md(1 hunks).changeset/shiny-paths-flow.md(1 hunks)packages/cache/redis/package.json(1 hunks)packages/cache/redis/src/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/shiny-paths-flow.md
🧰 Additional context used
🧬 Code graph analysis (1)
packages/cache/redis/src/index.ts (2)
packages/legacy/types/src/index.ts (1)
Logger(202-211)packages/legacy/types/src/pubsub.ts (3)
MeshPubSub(14-24)HivePubSub(6-6)toMeshPubSub(164-175)
⏰ 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). (13)
- GitHub Check: release / snapshot
- GitHub Check: e2e / node v22
- GitHub Check: integration / node 20
- GitHub Check: unit / node 22
- GitHub Check: integration / node 22
- GitHub Check: apollo-federation-compatibility
- GitHub Check: e2e / node v18
- GitHub Check: integration / node 18
- GitHub Check: unit / node 20
- GitHub Check: check
- GitHub Check: e2e / node v20
- GitHub Check: unit / node 18
- GitHub Check: deployment
🔇 Additional comments (1)
.changeset/@graphql-mesh_cache-redis-8830-dependencies.md (1)
1-5: Changeset looks good; confirm feature changeset existsPatch for dependency addition is fine. If OTEL tracing is a user-visible feature, ensure there’s a separate changeset entry (likely minor) describing the tracing spans for init/get/set/delete so release notes are accurate.
| const urlStr = redisUrl.toString(); | ||
| options.logger.debug(`Connecting to Redis at ${urlStr}`); | ||
| this.client = new Redis(urlStr); | ||
| } else { |
There was a problem hiding this comment.
Do not log Redis credentials (secret leakage)
Logging urlStr may include username/password from options.url. Sanitize before logging.
Apply this diff:
- const urlStr = redisUrl.toString();
- options.logger.debug(`Connecting to Redis at ${urlStr}`);
- this.client = new Redis(urlStr);
+ const urlStr = redisUrl.toString();
+ // Sanitize credentials for logs
+ const logUrl = new URL(urlStr);
+ if (logUrl.password) {
+ logUrl.password = '***';
+ }
+ options.logger.debug(`Connecting to Redis at ${logUrl.toString()}`);
+ this.client = new Redis(urlStr);📝 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.
| const urlStr = redisUrl.toString(); | |
| options.logger.debug(`Connecting to Redis at ${urlStr}`); | |
| this.client = new Redis(urlStr); | |
| } else { | |
| const urlStr = redisUrl.toString(); | |
| // Sanitize credentials for logs | |
| const logUrl = new URL(urlStr); | |
| if (logUrl.password) { | |
| logUrl.password = '***'; | |
| } | |
| options.logger.debug(`Connecting to Redis at ${logUrl.toString()}`); | |
| this.client = new Redis(urlStr); | |
| } else { |
🤖 Prompt for AI Agents
packages/cache/redis/src/index.ts lines 96-99: the code logs the full redis URL
(urlStr) which can contain username/password and leak secrets; instead, before
logging build a sanitized string that strips or redacts credentials (e.g., parse
the URL and omit username/password, or replace them with ****, or log only host
and port), use that sanitized value in options.logger.debug while still using
the original urlStr when creating this.client = new Redis(urlStr).
1 participant
🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant
motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
expected)
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take
action on it as appropriate
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration
Test Environment:
@graphql-mesh/...:Checklist:
CONTRIBUTING doc and the
style guidelines of this project
changeset using
yarn changesetthat bumps the versionFurther comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose
the solution you did and what alternatives you considered, etc...