Skip to content
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

fix(server_family): GetMetrics should show commands in lowercase #2601

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

lsvmello
Copy link
Contributor

Closes #862

I've ran INFO ALL locally and the only section that showed IN CAPS was cmdstat_COMMAND.

So I 'lowercased' all commands on GetMetrics() and fixed the replication_test.py accordingly.
Also checked that Redis behaves the same.

kostasrim
kostasrim previously approved these changes Feb 19, 2024
Copy link
Contributor

@kostasrim kostasrim left a comment

Choose a reason for hiding this comment

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

Hi @lsvmello! Thank you for your contribution 🚀

I left a very minor comment.

@dranikpg let me know if you had something else in mind with #862

src/server/server_family.cc Outdated Show resolved Hide resolved
@dranikpg
Copy link
Contributor

I don't know if we want all command stats to be in lowercase, I mean that we use(d) inconsistent naming for sections in INFO ALL. Some were camel (Transaction), come were caps (STATS, MEMORY), etc

@kostasrim
Copy link
Contributor

I don't know if we want all command stats to be in lowercase, I mean that we use(d) inconsistent naming for sections in INFO ALL. Some were camel (Transaction), come were caps (STATS, MEMORY), etc

Oh I see and I also did notice that this was an old issue. Anyway, their changes are only for two command stats:

# Commandstats         
cmdstat_COMMAND:calls=2,usec=50894,usec_per_call=25447             
cmdstat_INFO:calls=1,usec=2043,usec_per_call=2043

I can live with lowercase COMMAND and INFO so I personally don't mind moving forward with this PR unless there is some strong objection.

Also looking on the camelCase or CAPS it seems now that the naming of each category is coherent (First letter capital followed by all lower case) so I guess we can close the issue as well?

Signed-off-by: Leonardo Mello <lsvmello@gmail.com>
@lsvmello
Copy link
Contributor Author

I've added the change requested and rebased the branch.

@kostasrim kostasrim self-requested a review February 19, 2024 16:49
@kostasrim kostasrim merged commit 07a8411 into dragonflydb:main Feb 19, 2024
7 checks passed
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.

INFO command output should use consistent letter cases
3 participants