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.

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

Extend LATENCY LATEST to add min / sum / cnt stats #1570

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
+22 −4
Open

Extend LATENCY LATEST to add min / sum / cnt stats #1570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/latency.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,17 @@ void latencyAddSample(const char *event, mstime_t latency) {
ts = zmalloc(sizeof(*ts));
ts->idx = 0;
ts->max = 0;
ts->min = 0;
ts->sum = 0;
ts->cnt = 0;
memset(ts->samples, 0, sizeof(ts->samples));
dictAdd(server.latency_events, zstrdup(event), ts);
}

if (latency > ts->max) ts->max = latency;
if (latency < ts->min || ts->min == 0) ts->min = latency;
ts->sum += latency;
ts->cnt++;

/* If the previous sample is in the same second, we update our old sample
* if this latency is > of the old one, or just return. */
Expand Down Expand Up @@ -612,11 +618,14 @@ void latencyCommandReplyWithLatestEvents(client *c) {
struct latencyTimeSeries *ts = dictGetVal(de);
int last = (ts->idx + LATENCY_TS_LEN - 1) % LATENCY_TS_LEN;

addReplyArrayLen(c, 4);
addReplyArrayLen(c, 7);
addReplyBulkCString(c, event);
addReplyLongLong(c, ts->samples[last].time);
addReplyLongLong(c, ts->samples[last].latency);
addReplyLongLong(c, ts->max);
addReplyLongLong(c, ts->min);
Copy link
Member

@madolson madolson Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I vote to remove min. It's basically the lowest value above min, I think sum and cnt are more useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you mean min is the lowest value above the configured latency-monitor-threshold.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will it be useful in scenarios where latency-monitor-threshold may be frequently adjusted? or the real latency is much larger than latency-monitor-threshold?

i am not sure about it actually, i am ok with removing it if you both think it is useless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only Madelyn thinks it's useless. I don't have an opinion about it. 😆

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty strongly opinionated about only adding fields we are sure are useful. If we aren't sure something is useful, I would prefer to defer adding it until someone brings it up or has a use case. I prefer to lean towards simplicity as much as possible. The fewer the fields exist, the less the end users have to think about.

Will it be useful in scenarios where latency-monitor-threshold may be frequently adjusted? or the real latency is much larger than latency-monitor-threshold?

I'm not sure what the min would tell you in that scenario.

addReplyLongLong(c, ts->sum);
addReplyLongLong(c, ts->cnt);
}
dictReleaseIterator(di);
}
Expand Down
3 changes: 3 additions & 0 deletions src/latency.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ struct latencySample {
struct latencyTimeSeries {
int idx; /* Index of the next sample to store. */
uint32_t max; /* Max latency observed for this event. */
uint32_t min; /* Min latency observed for this event. */
uint32_t sum; /* Sum latency observed for this event. */
uint32_t cnt; /* Count observed for this event. */
struct latencySample samples[LATENCY_TS_LEN]; /* Latest history. */
};

Expand Down
12 changes: 9 additions & 3 deletions tests/unit/latency-monitor.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,18 @@ tags {"needs:debug"} {
puts $res
}

# See the previous "Test latency events logging" test for each call.
foreach event $res {
lassign $event eventname time latency max
lassign $event eventname time latency max min sum cnt
assert {$eventname eq "command"}
if {!$::no_latency} {
assert {$max >= 450 & $max <= 650}
assert {$time == $last_time}
# To avoid timing issues, each event decreases by 50 and
# increases by 150 to increase the range.
assert_equal $time $last_time
assert_range $max 450 650 ;# debug sleep 0.5
assert_range $min 250 450 ;# debug sleep 0.3
assert_range $sum 1050 1650 ;# debug sleep 0.3 + 0.4 + 0.5
assert_equal $cnt 3
}
break
}
Expand Down
Loading