Oh I think I'd better count flows for base/incr sync too.

I recently built some grafana dashboard that plots such graphs.
i monitored the increase over time of these counters:

• total_net_output_byte
• total_net_input_bytes
• master_repl_offset

The instantaneous metrics are nice if you just look at a single INFO output with redis-cli, so you have a point here.
However, your PR handles the RDB portion of the replication, but missing the live command stream if i'm not mistaken.

p.s. i don't think i like to change the meaning of the existing fields (backwards compatibility), so they should keep including the replication data too, and users will have to deduct on their own

Hi @oranagra , I just made some updates, now This PR:

1. adds 4 variables:
   server.stat_net_output_bytes_sync_fullresync/server.stat_net_input_bytes_sync_fullresync: for fullresync flow count
   server.stat_net_output_bytes_sync_cmds/server.stat_net_input_bytes_sync_cmds: for living command stream
2. adds 8 info stats

total_net_input_bytes_sync_fullresync:0
total_net_output_bytes_sync_fullresync:0
total_net_input_bytes_sync_cmds:0
total_net_output_bytes_sync_cmds:0
instantaneous_input_sync_fullresync_kbps:0.00
instantaneous_output_sync_fullresync_kbps:0.00
instantaneous_input_sync_cmds_kbps:0
instantaneous_output_sync_cmds_kbps:0.00

3. now server.stat_net_output_bytes/server.stat_net_input_bytes would keep the same meaning as before. If users would like to know the pure user's data flow count, they could deduct on their own.

Also here is a test example with a 2-node redis(1 master and 1 slave).

Steps:

1. start a new master node 30001
2. using redis-benchmark to write to 30001
   ./src/redis-benchmark -t set -r 10000000 -d 1 -n 100000000 -p 30001
3. after a while, start a new slave node 30002, run slaveof 127.0.0.1 30001
4. run info stats to check the flow count of the master and slave

Results:

• 30001 (master)

127.0.0.1:30001> info stats
# Stats
...
total_net_input_bytes:107794784
total_net_output_bytes:79574499
total_net_input_bytes_sync_fullresync:0
total_net_output_bytes_sync_fullresync:30543150
total_net_input_bytes_sync_cmds:0
total_net_output_bytes_sync_cmds:36493614
instantaneous_input_kbps:0.03
instantaneous_output_kbps:0.00
instantaneous_input_sync_fullresync_kbps:0.00
instantaneous_output_sync_fullresync_kbps:0.00
instantaneous_input_sync_cmds_kbps:0.00
instantaneous_output_sync_cmds_kbps:0.00
...

• 30002(slave)

...
127.0.0.1:30002> info stats
# Stats
...
total_net_input_bytes:67037001
total_net_output_bytes:54822
total_net_input_bytes_sync_fullresync:30543150
total_net_output_bytes_sync_fullresync:0
total_net_input_bytes_sync_cmds:36493614
total_net_output_bytes_sync_cmds:0
instantaneous_input_kbps:0.00
instantaneous_output_kbps:0.05
instantaneous_input_sync_fullresync_kbps:0.00
instantaneous_output_sync_fullresync_kbps:0.00
instantaneous_input_sync_cmds_kbps:0.00
instantaneous_output_sync_cmds_kbps:0.00
...

Also I think one thing about the second commit could be discussed here.
To keep the total_net_output_bytes_sync_fullresync of master as same as total_net_input_bytes_sync_fullresync of slave during replication, I also count some missed net stats:

1. count \n command at presync stage;
2. connSyncReadLine() would lose 1 byte for a line of string ended with \r\n, so I add 1 extra byte for nread when count this.

Not sure if these two things are elegant here, just to keep consistency.

Thank you for the review. @oranagra
And here is the code after revision.

1. Now I only keep stat_net_repl_input_bytes / stat_net_repl_output_bytes for replication stats.
2. stat_net_input_bytes/ stat_repl_output_bytes would only include repl bytes in info command.
3. fix nwritten type from int to ssize_t.

output of info stats is like

• master

# Stats
total_connections_received:53
total_commands_processed:213568
instantaneous_ops_per_sec:1
total_net_input_bytes:9183348
total_net_output_bytes:5301699
total_net_repl_input_bytes:0
total_net_repl_output_bytes:4226010
instantaneous_input_kbps:0.05
instantaneous_output_kbps:0.00
instantaneous_input_repl_kbps:0.00
instantaneous_output_repl_kbps:0.00
rejected_connections:0

• replica

total_connections_received:1
total_commands_processed:5
instantaneous_ops_per_sec:0
total_net_input_bytes:4226153
total_net_output_bytes:4347
total_net_repl_input_bytes:4226010
total_net_repl_output_bytes:0
instantaneous_input_kbps:0.01
instantaneous_output_kbps:0.05
instantaneous_input_repl_kbps:0.01
instantaneous_output_repl_kbps:0.00
rejected_connections:0

Updated. @oranagra
Now info output is like：

• for disk-based replication

  • master

  # Stats
  total_net_input_bytes:21534385
  total_net_output_bytes:24777743
  total_net_repl_input_bytes:0
  total_net_repl_output_bytes:22270070
  

  • slave

  # Stats
  total_net_input_bytes:22270213
  total_net_output_bytes:4020
  total_net_repl_input_bytes:22270070
  total_net_repl_output_bytes:0
  

• for diskless loading (by setting repl-diskless-load to swapdb in redis.conf)

  • master

   # Stats
  total_net_input_bytes:8984484
  total_net_output_bytes:26999927
  total_net_repl_input_bytes:0
  total_net_repl_output_bytes:25945330
  

  • slave

  # Stats
  total_net_input_bytes:25945448
  total_net_output_bytes:2769
  total_net_repl_input_bytes:25945330
  total_net_repl_output_bytes:0
  

@DarrenJiang13 can you please also make a redis-doc PR?

@oranagra finished. Added some words to redis-doc in info.md.

@soloestoy the way i see it (please tell me if you see it differently) these instantaneous metrics are just for human eyes.
real monitoring systems, take a snapshot of the INFO every second, and can measure the increase of the raw metrics (non-instantaneous) over time.
so for instance if you have a grafana dashboard showing redis metrics, it'll still be able to show the difference between input and output, and it'll also be accurate, since it doesn't depend on these (inaccurate) "instantaneous" metrics (corresponding to 1.6 seconds or so).

so considering that these metrics are just for a person taking one snapshot of INFO via redis-cli to examine it in the console, do we really need another metric for this case?

@oranagra we don't use grafana, our monitor system depends the "instantaneous" metrics, and as I know some users in our platform use the redis' "instantaneous" metrics too.

grafana or no grafana, i think a monitoring system should not use these.
i think think it's a bad metric that should only be used by user that takes one snapshot from command line, and that real monitoring systems need to compute deltas from raw data they capture from redis.
but if users use it, let's give it to them..
one of you want to make a PR to re-fix this ASAP?

p.s. i'm ok with that mainly because it's already there.. but if someone would suggest new metrics like instantaneous_connections_per_sec or any other metric like that, i'll probably argue against it.

come to think of it, someone could suggest that since we have instantaneous_ops_per_sec, we should split that to ones coming from replication stream, vs normal clients (instantaneous_repl_ops_per_sec). and won't like that.
maybe we should drop instantaneous_repl_total_kbps too?
@yossigo @madolson WDYT?

it's ok if we don't support new instantaneous metrics, but now for instantaneous_repl_total_kbps it's better to separate it, @DarrenJiang13 can you make a PR?

BTW, the instantaneous metrics help me a lot when troubleshooting problems, our monitor system is not good enough, especially it's hard to add a new item 😓

@oranagra I am not against the instantaneous metrics, I think they're useful for basic troubleshooting if you don't have a better monitoring system so I'm OK with adding that. But if we decide we need a new metric, the first choice should always be a raw value for proper monitoring systems to use.

I'm not against them either. They're useful from command prompt, but I wouldn't want to add dozens of them, one for every other raw metric we have.
That's why I thought it was enough to add just one instead of two..
But if others think it's necessary, I won't block that..

I'm not sure this specific one will help a lot for troubleshooting, e.g. more than like forks per second, or error reply rate, new client connections per second, etc.
I thought that one metric to show all replication related traffic would be enough to let the viewer realize that there's a lot of replication traffic, and they can do the rest of the investigation with raw metrics.

If I had to add one new instantaneous metric, I'd probably go for new client connection rate rather than add breakdown for replication in/out.

I agree that I think the instantaneous metrics should really only be used for debugging. AWS relies on incremental metrics for computing metrics we expose to end users so that we can compute deltas if we miss data points if the engine is busy or something.

@yossigo @madolson I'm not sure how to interpret your comments. Are you in favor of merging #10810 or do you think one new instantaneous replication bandwidth metric is enough to realize that replication is or isn't the cause of high traffic.

@soloestoy did we convince you that these are no good for real monitoring anyway?
I wanna stress that we're not discussing removing the existing old metrics, just the addition of new ones, so whoever used to rely on the old ones, is not in a worse state by the addition of the single new instantaneous metric instead of 2 .

@oranagra I would prefer to not add the instantaneous metrics. I think just looking at the deltas should be enough to determine where the resource utilization is.

@soloestoy did we convince you that these are no good for real monitoring anyway?

Nope : ) because I cannot convince our users, since they prefer instantaneous metrics.

I agree we don't add new instantaneous metrics from now on. But about instantaneous_repl_total_kbps, I strongly believe it's better to separate it.

@soloestoy Do you know how they are ingesting these instantaneous metrics? It seems weird that folks would be looking at this instead of periodically polling and doing math, but if users have a reason for it then I'm okay splitting it. It just seems like an anti-pattern to me.

because these instantaneous metrics are very easy to collect and display, no need to do math. And sometimes users don't collect metrics every second (maybe collect every minute), so they can only rely on instantaneous metrics.

Nope : ) because I cannot convince our users, since they prefer instantaneous metrics.

The way to convince them is that they won't have any other choice 8-). but besides, this is a new metrics that they didn't have till now anyway.

And sometimes users don't collect metrics every second (maybe collect every minute), so they can only rely on instantaneous metrics.

i think it's the other way around. if you take two samples 1 minute apart, the instantaneous metric won't tell you what happened in between (just in the last 1.6 seconds), but computing deltas would!.
i think the instantaneous is only useful when you take just one reading and without any tools (redis-cli).

but anyway, i think we've invested enough energy on this discussion, i think it's clear where we're going with other metrics, but let's merge that PR and put it behind us, maybe the excuse for me would be symmetry (i.e. the fact that all other metrics count input and output separately).