Add last_fork_start_time to INFO STATS #978
Conversation
Sometimes we want to know the last fork time, this can help user to analyze the stability since right now fork is still a costly operation and user can monitor forks by drawing graphs based on these fork fields. Signed-off-by: Binbin <[email protected]>
|
Currently the time is, a successful fork, when it start, is recorded. We can also record the last failed fork time if needed. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #978 +/- ##
============================================
- Coverage 70.66% 70.65% -0.02%
============================================
Files 114 114
Lines 63150 63152 +2
============================================
- Hits 44624 44619 -5
- Misses 18526 18533 +7
|
src/server.c
Outdated
| @@ -6304,6 +6306,7 @@ int serverFork(int purpose) { | |||
| server.stat_fork_time = ustime() - start; | |||
| server.stat_fork_rate = | |||
| (double)zmalloc_used_memory() * 1000000 / server.stat_fork_time / (1024 * 1024 * 1024); /* GB per second. */ | |||
| server.stat_last_fork_time = start; | |||
There was a problem hiding this comment.
Not sure I understand why server.stat_last_fork_time is needed if we already have server.stat_fork_time. (I'm assuming what you really want to know is how long fork took, which can be quite long with large datasets.)
There was a problem hiding this comment.
I'm assuming what you really want to know is how long fork took, which can be quite long with large datasets
we already has this info: latest_fork_usec: Duration of the latest fork operation in microseconds
here this latest_fork_time is the time when the last fork was triggered, something like that
There was a problem hiding this comment.
Could you clarify how this info is useful ? Would tracking the bgsave start time be more informative? Also, renaming it to last_fork_start_time or last_fork_start_timestamp might avoid confusion with server.stat_fork_time.
There was a problem hiding this comment.
we have other ways that can trigger fork, like aofrw, tracking the bgsave start time maybe not enought.
fork is still a very resource-intensive process, so it makes sense for me to record the time each fork is triggered.
There was a problem hiding this comment.
Understood.
I'd suggest renaming the variable as mentioned above; other than that, the code looks fine.
There was a problem hiding this comment.
good naming idea, sorry for the confusion. (bad english, unable to think of a good name right away...)
Signed-off-by: Binbin <[email protected]>
enjoy-binbin
changed the title
Signed-off-by: Binbin <[email protected]>
Signed-off-by: Binbin <[email protected]>
yes.. sorry for that, i did not check after push |
Sometimes we want to know the last fork start time, this can help
user to analyze the stability since right now fork is still
a costly operation and user can monitor forks by drawing graphs
based on these fork fields.
fork is still a very resource-intensive process, so it makes sense
to record the time each fork is triggered. Adding a last_fork_start_time
field in INFO STATS, to record the last fork start time in us.