Skip to content

show sub-commands INFO latencystats and LATENCY HISTOGRAM #10103

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.

Sign up for GitHub

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

Jump to bottom
Merged
oranagra merged 6 commits into from
Jan 17, 2022
Merged

show sub-commands INFO latencystats and LATENCY HISTOGRAM #10103

oranagra merged 6 commits into from
Jan 17, 2022

Conversation

@soloestoy
Copy link
Contributor

@soloestoy soloestoy commented Jan 12, 2022 •
edited by oranagra
Loading

since info commandstats already shows sub-commands, we should do the same in info latencystats.
similarly, the LATENCY HISTOGRAM command now shows sub-commands (with their full name) when:

  • asking for all commands
  • asking for a specific container command
  • asking for a specific sub-command)

@filipecosta90 filipecosta90 added the action:run-benchmark Triggers the benchmark suite for this Pull Request label Jan 12, 2022
oranagra
oranagra approved these changes Jan 12, 2022
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

good catch!

@oranagra
Copy link
Member

oranagra commented Jan 12, 2022

we also need to modify latencyAllCommandsFillCDF to use getFullCommandName, and add a test.
@soloestoy can you add that to this PR?

@soloestoy
Copy link
Contributor Author

soloestoy commented Jan 12, 2022

we also need to modify latencyAllCommandsFillCDF to use getFullCommandName, and add a test. @soloestoy can you add that to this PR?

OK, and latencySpecificCommandsFillCDF, that should be all.

oranagra
oranagra reviewed Jan 12, 2022
View reviewed changes
oranagra
oranagra approved these changes Jan 14, 2022
View reviewed changes
@enjoy-binbin enjoy-binbin mentioned this pull request Jan 15, 2022
Merged
1 task
@oranagra
cleanup and improve test code
466bf66 
sorry.. i didn't like the fact latency_histogram just took the second
element without matching the name of the command (first list element).

i didn't like the fact we repeadadly call the same command to validate
different things on the same output. (this also messed up the count when
trying to validate the count)

adding validation of the sub-command name when calling the unfiltered
LATENCYH HISTOGRAM
@oranagra
avoid spellcheck false positive
ce2e44c
@oranagra
Copy link
Member

oranagra commented Jan 17, 2022

@filipecosta90 @soloestoy FYI see the test cleanup i did in 466bf66 (converting the response to a dict and doing explicit lookups)

@oranagra oranagra merged commit 90916f1 into redis:unstable Jan 17, 2022
@oranagra oranagra changed the title show subcommands latencystats show sub-commands INFO latencystats and LATENCY HISTOGRAM Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

@filipecosta90 filipecosta90 Awaiting requested review from filipecosta90

Assignees

No one assigned

Labels

action:run-benchmark Triggers the benchmark suite for this Pull Request

Projects

Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@soloestoy @oranagra @filipecosta90