Documentation: Fix etcdHighNumberOfFailedGRPCRequests rules return NaN value

[qingwave]

what changed?
Rules etcdHighNumberOfFailedGRPCRequests in etcd3_alert.rules.yml

why this change was made?
The original rules will return NaN value of etcdHighNumberOfFailedGRPCRequests, but it should be a zero value. as image

The denominator sometimes is zero，and return a NaN value of rules. Replace division with comparison, as image

[spzala]

@qingwave thanks for the PR. Please make sure you have commit title in this format <packagename>: <description> e.g. Doc: fix etcdHighNumberOfFailedGRPCRequests rules

[codecov-io]

Codecov Report

Merging #10629 into master will decrease coverage by 0.32%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10629      +/-   ##
==========================================
- Coverage   71.69%   71.36%   -0.33%     
==========================================
  Files         393      393              
  Lines       36627    36628       +1     
==========================================
- Hits        26258    26140     -118     
- Misses       8532     8648     +116     
- Partials     1837     1840       +3

Impacted Files	Coverage Δ
pkg/adt/interval_tree.go	78.07% <0%> (-12.92%)	⬇️
auth/simple_token.go	77.23% <0%> (-9.76%)	⬇️
etcdctl/ctlv3/command/role_command.go	62.9% <0%> (-8.07%)	⬇️
etcdserver/api/v3rpc/lease.go	69.31% <0%> (-7.96%)	⬇️
pkg/netutil/netutil.go	63.11% <0%> (-6.56%)	⬇️
pkg/logutil/zap_grpc.go	47.61% <0%> (-4.77%)	⬇️
proxy/grpcproxy/watcher.go	85.71% <0%> (-4.09%)	⬇️
etcdserver/v2_server.go	80.76% <0%> (-3.85%)	⬇️
pkg/testutil/recorder.go	77.77% <0%> (-3.71%)	⬇️
clientv3/leasing/txn.go	88.09% <0%> (-3.18%)	⬇️
... and 19 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7c6894...af6aeb7. Read the comment docs.

[spzala]

@qingwave you need to update the commit title. The build failure gives this error - e932113 fix etcdHighNumberOfFailedGRPCRequests rules... Expected commit title format '<package>{", "<package>}: <description>' Got: e932113 fix etcdHighNumberOfFailedGRPCRequests rules Other than that per the output of the fix you have provided the changes looks good to me. Thanks!

[qingwave]

@spzala okay, the commit title modified

[spzala]

@qingwave this should be changed as well https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/etcd3_alert.rules#L41 Also I might be missing something but the example diagram you have provided for NaN seems not have the exact expression as what's in the current rules? Thanks!

[spzala]

@spzala okay, the commit title modified

Thanks, that has fixed the build.

[spzala]

/cc @xiang90

[qingwave]

@qingwave this should be changed as well https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/etcd3_alert.rules#L41 Also I might be missing something but the example diagram you have provided for NaN seems not have the exact expression as what's in the current rules? Thanks!

@spzala As your said, the fixed rules cannot return a exact expression, only 0 or 1. In prometheus, there is not a gracefully method to avoid divide by zero. We need to check for zero in divider and replace NaN with zero, as follows，

100 * sum(rate(grpc_server_handled_total{job=~".*etcd.*", grpc_code!="OK"}[5m])) BY (job, instance, grpc_service, grpc_method)
      /
sum(rate(grpc_server_handled_total{job=~".*etcd.*"}[5m])) BY (job, instance, grpc_service, grpc_method) > 0
      or 
sum(rate(grpc_server_handled_total{job=~".*etcd.*", grpc_code!="OK"}[5m])) BY (job, instance, grpc_service, grpc_method) > bool 0

Is it a good way?

[spzala]

@qingwave thanks and in that case it sounds good to me. Let's have @hexfusion or @xiang90 take a look too. Also, as I commented earlier the etcd3_alert.rules should be updated accordingly as well. Thanks!

[brancz]

I'm not sure this is the right fix. As far as I can tell the reason this is happening is because we're dividing an empty vector by a scalar, for which not a number is indeed correct. What exactly needs to be fixed here? Are alerts firing that shouldn't be firing?

[servo1x]

We're seeing this alert firing constantly, should this rule be removed entirely?

openshift/cluster-monitoring-operator#248 (comment)
#10289

[brancz]

@servo1x for what it's worth it's disabled in openshift (v4.1.0+) until we resolve this.