-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
feat(microservices): add redis driver identification #16218
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.
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
base: master
Are you sure you want to change the base?
feat(microservices): add redis driver identification #16218
Conversation
Pull Request Test Coverage Report for Build 93e42ae7-4231-4fa2-93ed-fafbab1018d4Details
💛 - Coveralls |
|
Instead of auto-retrieving the @nestjs/microservices packagae version, we could just add the |
@kamilmysliwiec, Thanks for the suggestion! Additionally, it auto-detects the NestJS version as a sensible default (e.g., {
transport: Transport.REDIS,
options: {
clientInfoTag: 'custom-tag'
}
} |
| } | ||
| } | ||
|
|
||
| protected getClientInfoTag(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please remove this (as suggested in the previous PR)? No need to auto-set it for everyone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kamilmysliwiec do you mean entirely removing the default value (e.g., nestjs_v11.1.12)? Wouldn't this result in having the client info tag as unset in most environments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine - it's been unset by default for +7 years now. We should let end users decide if they want tags to be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Client info is already being sent in recent node-redis versions following Redis recommendation for CLIENT SETINFO
Client libraries are expected to pipeline this command after authentication on all connections
We're not introducing new data transmission here, just enhancing the existing client info with upstream framework name following the official guide.
Currently: node-redis
With this change: node-redis(nestjs)
This is important information for Redis maintainers to optimize for specific frameworks, fix framework-specific issues, and debug production systems.
Users can still override the clientInfoTag value if they wish to erase framework information (note that node-redis/ioredis client info would still be sent by the driver libraries regardless of this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current default (node-redis) seems fine though, what's the reason for adding the (nestsjs) suffix there? even if you have multiple nestjs services, you'd end up with duplicated tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding upstream library information (name + version) helps Redis administrators distinguish logical clients, not just driver instances.
In environments where multiple services share a Redis instance, (nestjs_v11.1.12) allows operators to tell apart different application stacks even when they use the same node-redis version — especially when services may be running different NestJS versions.
This follows the Redis CLIENT SETINFO recommendation to identify both the client library and the upstream framework, and does not replace or duplicate node-redis’s own identification — it augments it.
Importantly, this remains overridable: users who don’t want framework-level identification can unset or customize clientInfoTag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but I'm just saying this should be opt in (as it was for years now), instead of opt out. Can you please update the PR to align with this (aka make it configurable but not set by default)? Whether it's useful for Redis administrators is an entirely separate matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the concern, but I think making this opt-in would significantly reduce its usefulness in practice.
If this were opt-in, most users would likely never configure it, which means the CLIENT SETINFO data would largely be missing — even though it’s primarily intended to help Redis operators rather than application authors. That’s why Redis recommends client libraries provide this information by default.
This also preserves existing behavior in node-redis, where client info is already sent in an opt-out fashion. We’re not changing that model here, just extending the information with upstream framework context in line with the Redis guidance.
For additional context, this pattern is also reflected elsewhere in the ecosystem. For example, fastify-redis appends framework-level client information by default, while still allowing users to override or disable it:
https://github.com/fastify/fastify-redis/blob/66ee62a1333c474d50c66a55c687628a94757f76/index.js#L24-L57
So overall, keeping this opt-out:
- maintains parity with
node-redis, - aligns with Redis
CLIENT SETINFOrecommendations, - and remains fully overridable for users who don’t want framework-level identification.
Happy to improve documentation or make the opt-out path more explicit if that helps address the concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please update it as suggested above, otherwise i won't be able to merge this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kamilmysliwiec
Would a simpler default of just nestjs (without the version) address your concern? This removes the auto-retrieval of the package version while still providing framework identification for Redis admins by default.
Users could still override (clientInfoTag: 'my-app') or disable (clientInfoTag: '') as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds Redis client identification metadata so Redis administrators can distinguish NestJS-originated connections (via ioredis microservices clients/servers and the sample Socket.IO adapter).
Changes:
- Added
clientInfoTagto the Redis (ioredis) options surface. - Set a default
clientInfoTagderived from the NestJS package version (with fallback), while allowing user override. - Added unit tests around
getClientInfoTag()and client creation paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sample/02-gateways/src/adapters/redis-io.adapter.ts | Sets clientInfoTag for the sample node-redis client and adds version-based tag helper. |
| packages/microservices/external/redis.interface.ts | Exposes clientInfoTag on IORedisOptions. |
| packages/microservices/client/client-redis.ts | Applies clientInfoTag when constructing ioredis clients and adds getClientInfoTag(). |
| packages/microservices/server/server-redis.ts | Applies clientInfoTag when constructing ioredis servers and adds getClientInfoTag(). |
| packages/microservices/test/client/client-redis.spec.ts | Adds tests for createClient() and getClientInfoTag(). |
| packages/microservices/test/server/server-redis.spec.ts | Adds tests for createRedisClient() and getClientInfoTag(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
Currently, when NestJS applications connect to Redis (via microservices or Socket.IO adapter), Redis administrators cannot easily identify which connections are from NestJS applications. The connections appear as generic
ioredisornode-redisclients without any framework identification.What is the new behavior?
This PR adds Redis driver identification by implementing the
clientInfoTagoption for both ioredis and node-redis clients. This allows Redis administrators to identify which framework (NestJS) and version is connecting to Redis.Changes:
clientInfoTagoption toIORedisOptionsinterface inpackages/microservices/external/redis.interface.tsgetClientInfoTag()method in:packages/microservices/client/client-redis.ts(ioredis client)packages/microservices/server/server-redis.ts(ioredis server)sample/02-gateways/src/adapters/redis-io.adapter.ts(node-redis adapter)getOptionsProp()helper for consistency with NestJS codebase conventionsBehavior:
package.jsonand setsclientInfoTagtonestjs_v{version}(e.g.,nestjs_v11.1.12)nestjsclientInfoTagin their Redis options:Result:
Redis connections will now be identified as:
ioredis(nestjs_v11.1.12)node-redis(nestjs_v11.1.12)This information is sent to Redis via the
CLIENT SETINFO LIB-NAMEcommand and can be viewed by Redis administrators usingCLIENT LISTorCLIENT INFO.Reference: https://redis.io/docs/latest/commands/client-info/
Does this PR introduce a breaking change?
Other information
Backward Compatibility: Older versions of ioredis or node-redis that don't support
clientInfoTagwill simply ignore the unknown option, ensuring backward compatibility.Testing:
getClientInfoTag()methodnpm run formatnpm run lint