feat(cache-redis): add OTEL traces #8830
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds OpenTelemetry tracing to @graphql-mesh/cache-redis (init, get, set, delete), 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.18)
note over Cache,OTEL: Initialization
App->>Cache: new RedisCache(options)
Cache->>OTEL: startActiveSpan("hive.cache.redis.init")
Cache->>Redis: connect (url/cluster/sentinel/host)
Redis-->>Cache: connected / error
Cache->>OTEL: end span
end
rect rgba(220,255,220,0.12)
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.12)
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.10)
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
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
⏰ 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)
🔇 Additional comments (2)
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: |
💻 Website PreviewThe latest changes are available as preview in: https://a0079b71.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.
EmrysMyrddin
force-pushed
the
feat/cache-redis/add-otel-traces
branch
from
4eeae94 to
9a31085
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/cache/redis/src/index.ts (4)
28-140: Solid initialization tracing with room for observability enhancement.The try/finally pattern correctly ensures the span ends on all code paths. Previous critical issues have been resolved.
Recommended improvement: Record exceptions on the span before ending to improve debugging:
}); } catch (e) { + span.recordException(e); + span.setStatus({ code: SpanStatusCode.ERROR, message: e.message }); + throw e; } finally { span.end(); }You'll need to import
SpanStatusCodefrom@opentelemetry/api.
147-160: Excellent span lifecycle management with opportunity for richer telemetry.The try/finally pattern and TTL conversion (seconds→milliseconds) are both correct. Previous critical span leak issue has been resolved.
Recommended enhancements for better observability:
set(key: string, value: V, options?: KeyValueCacheSetOptions): Promise<any> { return this.tracer.startActiveSpan('hive.cache.set', async span => { + span.setAttribute('cache.key', key); + if (options?.ttl) { + span.setAttribute('cache.ttl', options.ttl); + } 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); } + } catch (e) { + span.recordException(e); + span.setStatus({ code: SpanStatusCode.ERROR }); + throw e; } finally { span.end(); } }); }
162-169: Promise chain correctly fixed and working.The
.finally()is now properly chained to the promise. Previous critical syntax error has been resolved.Optional enhancement for debugging cache misses and errors:
get(key: string): Promise<V | undefined> { - return this.tracer.startActiveSpan('hive.cache.get', span => + return this.tracer.startActiveSpan('hive.cache.get', span => { + span.setAttribute('cache.key', key); + return this.client .get(key) .then(value => (value != null ? JSON.parse(value) : undefined)) + .catch(e => { + span.recordException(e); + span.setStatus({ code: SpanStatusCode.ERROR }); + throw e; + }) - .finally(() => span.end()), - ); + .finally(() => span.end()); + }); }
175-184: Clean delete operation with proper span lifecycle.The promise chain correctly ensures the span ends. The error handler at line 181 silently converts errors to
false, which appears intentional for delete operations.Optional enhancement for cache key tracking:
delete(key: string): Promise<boolean> { - return this.tracer.startActiveSpan('hive.cache.delete', span => + return this.tracer.startActiveSpan('hive.cache.delete', span => { + span.setAttribute('cache.key', key); + return this.client .del(key) .then( value => value > 0, () => false, ) - .finally(() => span.end()), - ); + .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(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/@graphql-mesh_cache-redis-8830-dependencies.md
- packages/cache/redis/package.json
- .changeset/shiny-paths-flow.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T17:11:35.160Z
Learnt from: enisdenjo
Repo: ardatan/graphql-mesh PR: 8716
File: packages/legacy/types/src/pubsub.ts:64-79
Timestamp: 2025-08-22T17:11:35.160Z
Learning: In the GraphQL Mesh codebase, when implementing MeshPubSub interfaces, the optional `options` parameter in the subscribe method may be intentionally omitted if it's unused and irrelevant for the specific implementation, even if it causes a minor TypeScript interface mismatch.
Applied to files:
packages/cache/redis/src/index.ts
📚 Learning: 2025-01-31T09:44:11.443Z
Learnt from: ardatan
Repo: ardatan/graphql-mesh PR: 8350
File: packages/cache/upstash-redis/src/index.ts:26-34
Timestamp: 2025-01-31T09:44:11.443Z
Learning: In GraphQL Mesh's KeyValueCache implementation, the `options.ttl` parameter in the `set` method is specified in seconds, which needs to be converted to milliseconds when using Redis's `px` option.
Applied to files:
packages/cache/redis/src/index.ts
🧬 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). (1)
- GitHub Check: unit / node 20
🔇 Additional comments (3)
packages/cache/redis/src/index.ts (3)
14-14: LGTM! Clean OTEL import.The import is correct and aligns with the new dependency on
@opentelemetry/api@^1.9.0.
23-23: LGTM! Proper field declaration.The private tracer field is correctly typed and will be initialized in the constructor.
203-209: Excellent security improvement - credentials properly sanitized.This helper correctly prevents credential leakage by redacting passwords before logging. The previous critical security issue has been thoroughly addressed.
Description
This PR adds OpenTelemetry integration to allow visualization of the impact of Redis calls on request execution time in traces.
Related to GW-463
Type of change
Please delete options that are not relevant.
Further comments
This should have a very low impact on performance on setups without OpenTelemetry setup, as the default Tracer is a no-op one.