i see few things i listed in the issue are missing, let's add them in the PR later on, after we're happy with the ones you already added.

• eventloop_duration_cron_sum - total time spent in serverCron, and beforeSleep (excluding writes and aof)
• instantaneous_eventloop_cycles - cycles per second in the last 1.6 seconds
• instantaneous_eventloop_duration_avg - average duration in the last 1.6 seconds
• instantaneous_eventloop_duration_max - max duration in the last 1.6 seconds

@oranagra

Is read (in beforeSleep) considered a part of cron?

And about the 3 instantaneous stats, are they updated every 1.6 seconds, or updated every loop? I think we can leave these to external monitoring tools, so the server can just provide basic stats.

@CharlesChen888 my idea for a "cron" metric was to measure the sum of active-expire, active-defrag and all other tasks done by cron and beforeSleep that are not counted by the other set of metrics (so we should exclude read, write and AOF from these).

regardign the "instantaneous" metrics, i agree they're unnecessary for people who have a proper monitoring software connected to redis. they're there for easy consumption from command prompt by using redis-cli.
i meant to add them to the STATS_METRIC_COUNT mechanism.

• eventloop_duration_cron_sum - total time spent in serverCron, and beforeSleep (excluding writes and aof)

agree, cron sum is useful, especially about the expire cycle and defrag cycle in serverCron.

moreover, we also have two other time events, moduleTimerHandler and evictionTimeProc, do you think we should record these two? @oranagra

• instantaneous_eventloop_cycles - cycles per second in the last 1.6 seconds
• instantaneous_eventloop_duration_avg - average duration in the last 1.6 seconds
• instantaneous_eventloop_duration_max - max duration in the last 1.6 seconds

I'm not sure if they're useful, and instantaneous...max these two words are a bit strange together.

Maybe you wanna record how many eventloop cycles in last xxx seconds, and average/max duration in the last xxx seconds. But I think it should take longer like 1 min, and we should introduce another way to record them instead of using the current instantaneous metric.

i don't think we wanna break down cron to each and every task it contains.
we already have latency monitor metrics for active expiry and defrag, so for the purpose of event loop monitoring i think it can be inclusive without a breakdown (other than IO which we exclude from it).
then if someone realizes the event loop takes long because of cron, they can dig into it with other ways.

regarding the instantaneous metrics, IIRC last time we argued about these metrics (in #10062), i argued that we don't need them and you argued that we do...
i do think we need to keep them to a minimum, maybe we should only include only instantaneous_eventloop_cycles, or none of them.
having just duration_avg without duration_max feels a bit odd to me, and having just cycles per second without any duration metric also feels a bit odd. if the problem is the contradiction of "instantaneous" and "max" terms, maybe we can add "recent".

regarding the duration being 1 second or 1 minute, i don't wanna introduce another "infrastructure" for this, so i'd rather keep the 1.6 seconds thing we have now.
under constant load, it would be sufficient to have them represent 1 second (will show same values as 1 minute), so let's keep that. in that case maybe having an average duration, and no max is actually ok.

bottom line, if you are ok with it, let's drop the max and add just two.

So do we have a final design for this? INFO or histogram? And what metrics to expose?

If we just show the metrics in info, we may have a new section INFO EVENTLOOP, and it contains the list below (may be drop the instantaneous_eventloop_duration_max).

eventloop_cycles
eventloop_duration_sum
eventloop_duration_max
eventloop_duration_cmd_sum
eventloop_duration_io_read_sum
eventloop_duration_io_write_sum
eventloop_duration_aof_sum
eventloop_duration_cron_sum

instantaneous_eventloop_cycles
instantaneous_eventloop_duration_avg 
instantaneous_eventloop_duration_max

If we want a histogram, we may have a new command (or perhaps a new subcommand under LATENCY?), and it returns the information below.

1) "eventloop"
2) 1) "count"
   2) (integer) 24
   3) "histogram_usec"
   4) 1) (integer) 132
      2) (integer) 18
      3) (integer) 264
      4) (integer) 24
3) "eventloop|commands"
4) 1) "count"
   2) (integer) 1
   3) "histogram_usec"
   4) 1) (integer) 4227
      2) (integer) 1
5) "eventloop|ioread"
6) 1) "count"
   2) (integer) 1
   3) "histogram_usec"
   4) 1) (integer) 4227
      2) (integer) 1
......

Or we show sum in info, and details in histogram?
@oranagra @madolson @soloestoy

i'm leaning towards keeping the simple metrics in INFO STATS (with the instantaneous but without the max), and exposing the histogram in LATENCY EVENTLOOP.

the histogram data itself in the event loop should probably be plain (similar to the LATENCY HISTOGRAM output), but maybe put under a "histogram" field in a map, so we can extend that map in the future.

regarding the instantaneous metrics, IIRC last time we argued about these metrics (in #10062), i argued that we don't need them and you argued that we do...

I need clear it, I mean instantaneous max metric is strange. IMHO instantaneous is always used on xxx per second metric, like qps for query per second and bandwidth for net bytes per second, I don't know how to understand "duration time per second".

i view the "instantaneous" as a metric representing the very recent time (last 1.6 seconds), it doesn't have to be a per-second metric.

instantaneous_ops_per_sec is a per-sec, but it doesn't mean there couldn't be an instantaneous_cpu_usage.
i.e. unlike used_memory_rss which represents the current state, CPU usage, and ops/sec represent delta of some metric over time, and we choose to show the delta over the last 1.6 seconds, i don't see a problem showing instantaneous_eventloop_duration_avg

i view the "instantaneous" as a metric representing the very recent time (last 1.6 seconds), it doesn't have to be a per-second metric.

seems you misunderstand the current instantaneous mechanism or I missed something? it is the average of the past 16 per-second metric samples, in another word it is indeed the per-second metric that just calculated as an average of 16 samples, nothing todo with 1.6 seconds.

/* Add a sample to the operations per second array of samples. */
void trackInstantaneousMetric(int metric, long long current_reading) {
    long long now = mstime();
    long long t = now - server.inst_metric[metric].last_sample_time;
    long long ops = current_reading -
                    server.inst_metric[metric].last_sample_count;
    long long ops_sec;

    ops_sec = t > 0 ? (ops*1000/t) : 0;

    server.inst_metric[metric].samples[server.inst_metric[metric].idx] =
        ops_sec;
    server.inst_metric[metric].idx++;
    server.inst_metric[metric].idx %= STATS_METRIC_SAMPLES;
    server.inst_metric[metric].last_sample_time = now;
    server.inst_metric[metric].last_sample_count = current_reading;
}

/* Return the mean of all the samples. */
long long getInstantaneousMetric(int metric) {
    int j;
    long long sum = 0;

    for (j = 0; j < STATS_METRIC_SAMPLES; j++)
        sum += server.inst_metric[metric].samples[j];
    return sum / STATS_METRIC_SAMPLES;
}

i think the confusion is that you're referring to the way they're implemented and i'm referring to the way they're presented to the user.
if we go my way, we can rename trackInstantaneousMetric to trackInstantaneousRateMetric, and add a similar trackInstantaneousAvgMetric.

i.e. the current mechanism takes a rate sample (ops / sec) once in 100ms, and then computes an average of these over 1.6 seconds.
but we can add another similar function that takes a sum and count samples every 100ms to compute an average (average duration in this case), and then compute an average for these over 1.6 seconds.

i'm saying 1.6 seconds because there are 16 samples and we take one every 100ms.

since this was stale for a while, i'll try to promote a minimal version, so the status from my perspective (if we put aside the histogram feature) what's missing is:

• add eventloop_duration_cron_sum - total time spent in serverCron, and beforeSleep (excluding writes and aof)
  agree, cron sum is useful, especially about the expire cycle and defrag cycle in serverCron.
• remove eventloop_duration_max for now.

IMO it could be nice to have two metrics for easy consumption from redis-cli (but not a must):

• instantaneous_eventloop_cycles - cycles per second in the last 1.6 seconds
• instantaneous_eventloop_duration_avg - average duration in the last 1.6 seconds
  to clarify (since i didn't see a response) we can rename trackInstantaneousMetric to trackInstantaneousRateMetric, and add a similar trackInstantaneousAvgMetric. getInstantaneousMetric can remain as is, and i don't think any of that contradicts the "instantaneous" concept from the user's perspective.

@CharlesChen888 can you handle these changes or maybe want me to try to step in and help?

@oranagra Sorry about the delay, I went on vacation last weekend and I will get back to this tomorrow.

full CI: https://github.com/redis/redis/actions/runs/4958856487

@CharlesChen888 please take a look at the test failures, some are thresholds that should be adjusted (take a safe distance, maybe twice of what's printed), and there's also some odd fortify compilation error that i didn't look into..
thanks.

tests are now passing, and i also verified that we don't see that fortification error in unstable. trying to figure out how it is related to this PR..