Python: Add CLIENT SETNAME to Valkey/Redis connector for connection identification#14039
Python: Add CLIENT SETNAME to Valkey/Redis connector for connection identification#14039kuriangeorgebinu wants to merge 3 commits into
Conversation
…dentification Fixes microsoft#14038 Summary: - Added client_name parameter to Redis.from_url() calls in RedisCollection class - Added client_name parameter to Redis.from_url() calls in RedisStore class - Client name set to 'semantic_kernel_vector_store_client' following the suggested naming pattern Why This Matters: When monitoring a Valkey server with multiple connected applications, CLIENT LIST shows each connection's name. Without a client name, operators cannot distinguish Semantic Kernel connections from other anonymous clients. This is especially important in production environments with ElastiCache where multiple services share the same Valkey cluster. Setting client_name sends a CLIENT SETNAME command on connection, making the connection identifiable in: - CLIENT LIST output - Monitoring dashboards (e.g., Valkey Admin) - CloudWatch metrics (ElastiCache)
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 92%
✓ Correctness
This is a straightforward, correct change that adds
client_nameto the twoRedis.from_url()calls where the library creates its own connection. Theredis.asyncio.client.Redis.from_url()accepts**kwargspassed to the connection pool/constructor, andclient_nameis a standard redis-py parameter that issuesCLIENT SETNAMEon connect. When users provide their ownredis_databaseinstance, it is passed through unchanged (correct behavior). No correctness issues found.
✓ Security Reliability
This PR adds a static client_name parameter ('semantic_kernel_vector_store_client') to two Redis.from_url() calls for connection identification purposes. The change is minimal, uses a hardcoded constant string (no injection risk), and doesn't alter any connection logic or error handling. No security or reliability issues identified.
✓ Test Coverage
The PR adds
client_name="semantic_kernel_vector_store_client"to twoRedis.from_url()calls (inRedisCollection.__init__andRedisStore.__init__), but includes no tests verifying the parameter is passed correctly. Existing unit tests intest_redis_store.pytest initialization of both classes (e.g.,test_vector_store_defaults,test_collection_init) and verify connection properties likehost, but never assertclient_name. Since the change is simple and unlikely to regress, this is a suggestion rather than a blocking issue.
✗ Design Approach
The change adds connection identification, but the current design hardcodes a single client name with no caller override. That is weaker than the existing configuration pattern used elsewhere in this repo for client/application identification, and it leaves all Semantic Kernel deployments reporting the same name instead of letting each service identify itself distinctly.
Flagged Issues
- The new
client_nameis hardcoded with no way for callers to override it (RedisCollection.__init__andRedisStore.__init__expose no parameter). The Cosmos connector in the same repo already follows a better pattern: acceptingapplication_namefrom the caller and threading it into the client constructor. Consider makingclient_nameconfigurable with the current literal as the default.
Automated review by kuriangeorgebinu's agents
Addresses PR feedback on issue microsoft#14038 Changes: - Added 'client_name' parameter to RedisCollection.__init__() with default - Added 'client_name' parameter to RedisStore.__init__() with default - Updated docstrings to document the new parameter - Follows the same pattern as Cosmos connector's 'application_name' parameter - Maintains backward compatibility with default value This allows callers to override the client name for their specific use case while providing a sensible default 'semantic_kernel_vector_store_client' for cases where custom naming is not required.
3d32bbb to
a0377ec
Compare
|
@kuriangeorgebinu please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
…dentification
Fixes #14038
Summary:
When monitoring a Valkey server with multiple connected applications, CLIENT LIST shows each connection's name. Without a client name, operators cannot distinguish Semantic Kernel connections from other anonymous clients. This is especially important in production environments with ElastiCache where multiple services share the same Valkey cluster. Setting client_name sends a CLIENT SETNAME command on connection, making the connection identifiable in:
Motivation and Context
Description
Contribution Checklist