Sitelet https://github.com/redis/node-redis/pull/3099
Skip to content

Add latency histogram - #3099

Merged
nkaradzhov merged 8 commits into
redis:masterfrom
NaughtySora:add-latency-histogram
Nov 3, 2025
Merged

Add latency histogram#3099
nkaradzhov merged 8 commits into
redis:masterfrom
NaughtySora:add-latency-histogram

Conversation

@NaughtySora

@NaughtySora NaughtySora commented Oct 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Adding LATENCY HISTOGRAM command.
I personally will use this in my current and future projects.
#1955

I have tried to run all the tests locally but facing the same issue with timeout as before.

  • Does npm test pass with this change (including linting)?
    Tests are present.
  • Is the new or changed code fully tested?
    I think documentation about commands located in redis official website.
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@jit-ci

jit-ci Bot commented Oct 10, 2025

Copy link
Copy Markdown

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

nkaradzhov
nkaradzhov requested changes Oct 13, 2025
View reviewed changes

@nkaradzhov nkaradzhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @NaughtySora, thanks again for the contribution! There are couple of issues, i have created a PR to your branch with fixes.

  1. Add some tests for RESP3
  2. Change histogram_usec to be a map

@NaughtySora

NaughtySora commented Oct 13, 2025

Copy link
Copy Markdown
Contributor Author

Hi, thank you so much for the amends.
I will be more careful next time with REST 2/3 and redis documentation.

@NaughtySora

NaughtySora commented Oct 14, 2025

Copy link
Copy Markdown
Contributor Author

I'm i right to assume that according to the redis documentation i need also implement RESP 2 transform function with histogram usec tuple?
I'm a bit confused after merging the PR.

@nkaradzhov
nkaradzhov self-requested a review October 21, 2025 08:39
@nkaradzhov

nkaradzhov commented Nov 3, 2025

Copy link
Copy Markdown
Collaborator

@NaughtySora sorry, i didnt quite get what you mean.

@nkaradzhov
nkaradzhov merged commit dae47b4 into redis:master Nov 3, 2025
17 checks passed
@NaughtySora

NaughtySora commented Nov 3, 2025

Copy link
Copy Markdown
Contributor Author

in transformReply object there are 2 options for RESP 2 and 3.
RESP 2 has undefined value, and 3 has an implementation according to redis documentation.
But redis documentation also says that RESP 2 has its own contract.
I thought i need to implement it as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nkaradzhov nkaradzhov nkaradzhov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@NaughtySora @nkaradzhov