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

add stat_fork_rate in stat information #4711

Closed
wants to merge 1 commit into from
Closed

Conversation

jiafu1115
Copy link

@antirez Thanks for your review

@trevor211
Copy link
Collaborator

What do you think about it? @oranagra
Is it designed to report every stat variables to info stats?

@oranagra
Copy link
Member

Truth be told, I never understood the purpose of this stat, the variable, its use in the LATENCY DOCTOR.
But i'm not sure it's a justification to remove it.
This PR is a bit shy on details..
@jiafu1115 please state why do you want to remove it.

@oranagra
Copy link
Member

Ohh, i now notice this PR is trying to add a stat, not remove it.
the word "lose" in the title confused me.
still, @jiafu1115 please describe why you wanna add this stat?

looking closer, this variable seems to measure the time it takes the fork syscall to clone the page table (GB per second).

@jiafu1115
Copy link
Author

jiafu1115 commented Sep 25, 2020

Ohh, i now notice this PR is trying to add a stat, not remove it.
the word "lose" in the title confused me.
still, @jiafu1115 please describe why you wanna add this stat?

looking closer, this variable seems to measure the time it takes the fork syscall to clone the page table (GB per second).

@oranagra yes. the PR is to add the stat instead of to remove it. Redis has the variable and can monitor it so I suggest to add the stat similar with other ones with little effort and risk. After all, all variables except this one had/can be monitored except this one so I use "lose" word instead of "add" in this PR.
What's more, the stat_fork_rate is one of important monitored items when performance test tunning or production performance monitor. We often store so many keys into the redis and want to know all the performance items which may influence application when trouble shooing.

BTW: I update the title to avoid confusing us. thanks

@jiafu1115 jiafu1115 changed the title lose stat_fork_rate in stat information add stat_fork_rate in stat information Sep 25, 2020
@oranagra
Copy link
Member

I don't see any value in that Stat, in order to add it, it needs to have a much better descriptive name, current name is very bad (which is ok for an internal variable that can be renamed anytime, but not for an API that needs to be steady).
Anyway, I don't see how this stat is useful, I think you're much better enable the latency monitor and you'll see for how long the fork freezes (in milliseconds rather than GB per second).

The fact that other stats variables are exposed is not a reason to add it.

@jiafu1115
Copy link
Author

jiafu1115 commented Sep 26, 2020

I don't see any value in that Stat, in order to add it, it needs to have a much better descriptive name, current name is very bad (which is ok for an internal variable that can be renamed anytime, but not for an API that needs to be steady).
Anyway, I don't see how this stat is useful, I think you're much better enable the latency monitor and you'll see for how long the fork freezes (in milliseconds rather than GB per second).

The fact that other stats variables are exposed is not a reason to add it.

got it. thank for your comments. "see for how long the fork freezes", what's the monitor item?

@oranagra oranagra closed this Sep 27, 2020
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.

None yet

3 participants