Skip to content

feat(client): implement LATENCY RESET command #3039

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

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

Conversation

watersRand
Copy link

Here's the modified Pull Request description, filling in the details based on our work and including the important note about the testing environment.

Adds the LATENCY RESET command to the client, including:
- `parseCommand` and `transformReply` logic.
- Comprehensive unit tests for argument transformation.
- An integration test to verify end-to-end functionality.
- Updates to `packages/client/lib/commands/index.ts` to expose the command.

### Description

This Pull Request introduces the `LATENCY RESET` command to the `node-redis` client. The `LATENCY RESET` command is used in Redis to clear latency samples for specific events or all events, which is crucial for monitoring and managing Redis performance.

**Purpose:**
The primary purpose of this feature is to provide `node-redis` users with the ability to programmatically reset Redis latency statistics, allowing for cleaner monitoring cycles or troubleshooting.

**Implementation Details:**
- Implemented `parseCommand` to correctly format the `LATENCY RESET` command with optional event arguments (e.g., `LATENCY RESET command`, `LATENCY RESET fork`).
- Implemented `transformReply` to handle the integer reply from Redis, representing the oldest timestamp of the reset events.
- Comprehensive unit tests (`LATENCY_RESET.spec.ts`) are included to ensure `parseCommand` correctly transforms arguments for various scenarios (no events, single event, multiple events).
- An integration test is provided to verify the end-to-end functionality of `client.latencyReset` against a live Redis server. This test sets a latency threshold, generates latency events, performs resets (both all and specific events), and verifies that latency reports are cleared as expected.

**Redis Documentation Reference:**
For more details on the `LATENCY RESET` command, refer to the official Redis documentation: [https://redis.io/commands/latency-reset/](https://redis.io/commands/latency-reset/)

**Testing Environment Note (Important!):**
During local development , I encountered persistent timeouts related to Docker container startup and client connection. To ensure the tests could run to completion, I temporarily modified the internal timeout within `node_modules/@redis/test-utils/dist/lib/index.js` (specifically, increasing `this.timeout` in the `testWithClient` hook from 30s to 3min). This modification was purely for my local testing environment and is **not part of this core contribution**. It should not be merged into the main codebase. The tests were successfully run against `redislabs/client-libs-test:8.2-rc1`.

---


Adds the LATENCY RESET command to the client, including:
- transformArguments and transformReply logic.
- Comprehensive unit tests for argument transformation.
- An integration test to verify end-to-end functionality.
- Updates to packages/client/lib/commands/index.ts to expose the command.
Copy link
Collaborator

@nkaradzhov nkaradzhov left a comment

Choose a reason for hiding this comment

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

Hi @watersRand, thanks for the contribution. In general the PR looks very good, just some minor adjustments needed.

Sidenote: It would be interesting to see your prompts :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of the comments here are unnecessary. Lets only keep the jsdocs

Copy link
Collaborator

Choose a reason for hiding this comment

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

this file should not be part of the PR

Comment on lines 7 to 9
// Set a generous timeout for the entire test suite.
// This is crucial for the Docker container to spin up and client to connect reliably.
this.timeout(300000); // 5 minutes
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this, its not needed in CI etc.

assert.deepEqual(latestLatencyAfterMultipleReset, [], 'Expected no latency events after multiple specified resets.');

}, {
// These options are passed to testUtils.testWithClient for setting up the Redis server and client.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary comment

}, {
// These options are passed to testUtils.testWithClient for setting up the Redis server and client.
...GLOBAL.SERVERS.OPEN,
clientOptions: { // Configure the client created by testWithClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary comment

...GLOBAL.SERVERS.OPEN,
clientOptions: { // Configure the client created by testWithClient
socket: {
connectTimeout: 300000 // Set client connection timeout to 5 minutes (300000ms)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants