Repository navigation
contrib/mixin/mixin.libsonnet: Adjust gRPC failed requests - #13127
Conversation
OK is not the only one that is allowed, this before also captured context canceled, NotFound, and other non error requests.
|
@hexfusion please have a look, thanks! |
| alert: 'etcdHighNumberOfFailedGRPCRequests', | ||
| expr: ||| | ||
| 100 * sum(rate(grpc_server_handled_total{%(etcd_selector)s, grpc_code!="OK"}[5m])) without (grpc_type, grpc_code) | ||
| 100 * sum(rate(grpc_server_handled_total{%(etcd_selector)s, grpc_code=~"Unknown|FailedPrecondition|ResourceExhausted|Internal|Unavailable|DataLoss|DeadlineExceeded"}[5m])) without (grpc_type, grpc_code) |
There was a problem hiding this comment.
What would be easier to maintain list of failed codes or successful ones? I lean towards keeping the list of successful codes, as it better handles case when new code is introduced. It assumes that new codes are errors, so it will alert by default which is better than assuming that new codes are ok and not noticing problem. WDYT?
There was a problem hiding this comment.
Sure can do that, I get your point, just not sure when we would be adding new codes, as these are the gRPC standard status codes listed here https://grpc.github.io/grpc/core/md_doc_statuscodes.html.
There was a problem hiding this comment.
@serathius let me know what you think, happy to reverse the order if you think it still makes sense here, thanks!
There was a problem hiding this comment.
I think it's ok as it doesn't look there will be any difference in cost of maintaining this list up to date.
There was a problem hiding this comment.
I know it's old, but haven't we left out codes that actually weren't creating the noise? E.g. in K8s, only the 'Canceled' code was really creating noise for me.
There was a problem hiding this comment.
Same here, only 'Canceled' is giving the noise
There was a problem hiding this comment.
+1 this cleans up a fair amount of noise, thanks @lilic. Regarding new codes we have not seen an addition to the list since the initial commit[1] so I think this approach is fine.
etcd-io/etcd#13127 Let me try to summarize the issue: The problem of the expression comes from the grpc_code!="OK" part: it treats other non-error codes (canceled, invalidArgument, notFound, and etc) as errors so the expression reports a high rate of errors thus triggers too many alerts (see GRPC standard here). The above problem in the expression has been fixed in the upstream etcd repo: etcd-io/etcd#13127 Signed-off-by: svparamzin <[email protected]>
The tenant etcd copy of etcdHighNumberOfFailedGRPCRequests counted every code other than OK. A Watch stream is counted once, when it ends, and the streams of a kube-apiserver that stops end with Canceled. So whenever an API server on that etcd goes away, the Watch method of every member reads as 100 percent failed and the critical rule fires on all of them. The rules now count the codes the etcd mixin counts since etcd-io/etcd#13127, the same list the platform copy in monitoring-agents already uses. The grouping by method stays, so the alert labels do not change. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]>
…ts (#4985) ## What this PR does The tenant etcd chart has its own copy of `etcdHighNumberOfFailedGRPCRequests`, and both of its rules count every `grpc_code` other than `OK` as a failure. `grpc_server_handled_total` counts a Watch stream once, when it ends, and the streams of a kube-apiserver that goes away end with `Canceled`. So whenever an API server on a tenant etcd stops, the Watch method of every member reads as 100 percent failed and the critical rule fires on all of them. Both rules now count only `Unknown|FailedPrecondition|ResourceExhausted|Internal|Unavailable|DataLoss|DeadlineExceeded`, the list the etcd mixin has used since etcd-io/etcd#13127 and the one the platform copy in `packages/system/monitoring-agents/alerts/etcd.yaml` already uses. The grouping `BY (job, instance, grpc_service, grpc_method)`, the thresholds, `for` and the annotations stay as they are, so the alerts keep their labels. Seen on a v1.6.5 cluster while a Kubernetes cluster on a tenant etcd was created and deleted a few minutes later. vmalert had the critical alert firing for `grpc_method="Watch"` on all three members with the value 100. At that point `rate(grpc_server_handled_total{grpc_code!="OK"}[10m])` was 0.63 for Watch with `Canceled`, and the only other non-OK codes were `FailedPrecondition` on `RoleGet` and `UserGet` at 0.0017 each, which is one call each. The same query an hour earlier returned nothing. The platform copy runs in the same vmalert over the same series, and none of the firing alerts came from it. Each alert also showed up seven times there, once per tenant copy of these rules, because the root tenant's vmalert loads the rules of every tenant namespace. This PR does not change that. `hack/etcd-grpc-alerts.bats` renders the chart's PrometheusRule with `helm template` and replays it through `vmalert-tool` v1.136.0 pinned by digest, the same image #4963 uses. `hack/testdata/etcd-grpc-alerts/unittest.yaml` has three cases with seven checks. Watch streams that end only with `Canceled` must not fire at 6, 12 and 30 minutes. `Range` calls failing with `Unavailable` at 25 percent must fire the critical rule at 7 minutes and both rules at 12, with the labels and the summary pinned. Both together must fire only for `Range`. Against the rules on main the suite fails 4 of the 7 checks, all of them on Watch alerts with the value 100. Dropping `Unavailable` from the list, or grouping by `job` and `instance` only, fails 3 checks each. ```console $ bats hack/etcd-grpc-alerts.bats 1..1 ok 1 tenant etcd failed gRPC request alerts pass their vmalert-tool cases ``` The same run on the rules from main prints `not ok 1` and the four failing checks. With Bats 1.14.0 at the commit CI pins, the suite passes together with `bats-strict-setup`, `bats-no-negated-assert`, `bats-no-exit-trap`, `bats-runner-coverage`, `bats-runner-contract`, `alert-severity-contract` and `image-pin-consistency`, 81 of 81. Like the migration suites it needs docker, and only the image pull touches the network. Helm 3.17.3 and 4.2.4 render the same rules. I have not run the new rules in vmalert on a live cluster. The rules file is the same on release-1.6 and release-1.7. The commit cherry-picks onto release-1.6 without conflicts, and that backport is a separate PR with one more commit that drops the `load test_helper` line, because release-1.6 runs `hack/*.bats` through `hack/cozytest.sh`. ### Screenshots Not a UI change. ### Downstream repositories A rule template in `packages/extra/etcd`, a Bats suite under `hack/` and its test data. No values, schema, README, image or `ApplicationDefinition` change, and the website does not mention these rules, so no trigger map entry applies. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(etcd): the tenant etcd alert etcdHighNumberOfFailedGRPCRequests now counts only the gRPC codes that the etcd mixin and the platform copy of the alert count, so the Canceled watch streams of a Kubernetes API server that stops no longer fire it on every etcd member. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Refined etcd gRPC failure alerts to target specific failure status codes, reducing alerts for non-failure responses while preserving existing thresholds and timing. - Added coverage for canceled watch streams, unavailable range requests, and combined failure scenarios, including warning and critical alert evaluations. - **Tests** - Added integration validation for rendering etcd alert rules and running them through the alert-testing tool, including failure handling when prerequisites are unavailable. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
The tenant etcd copy of etcdHighNumberOfFailedGRPCRequests counted every code other than OK. A Watch stream is counted once, when it ends, and the streams of a kube-apiserver that stops end with Canceled. So whenever an API server on that etcd goes away, the Watch method of every member reads as 100 percent failed and the critical rule fires on all of them. The rules now count the codes the etcd mixin counts since etcd-io/etcd#13127, the same list the platform copy in monitoring-agents already uses. The grouping by method stays, so the alert labels do not change. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]> (cherry picked from commit 1ca7c0d) Signed-off-by: cozystack-ci[bot] <274107086+cozystack-ci[bot]@users.noreply.github.com>
…ts (#4985) ## What this PR does The tenant etcd chart has its own copy of `etcdHighNumberOfFailedGRPCRequests`, and both of its rules count every `grpc_code` other than `OK` as a failure. `grpc_server_handled_total` counts a Watch stream once, when it ends, and the streams of a kube-apiserver that goes away end with `Canceled`. So whenever an API server on a tenant etcd stops, the Watch method of every member reads as 100 percent failed and the critical rule fires on all of them. Both rules now count only `Unknown|FailedPrecondition|ResourceExhausted|Internal|Unavailable|DataLoss|DeadlineExceeded`, the list the etcd mixin has used since etcd-io/etcd#13127 and the one the platform copy in `packages/system/monitoring-agents/alerts/etcd.yaml` already uses. The grouping `BY (job, instance, grpc_service, grpc_method)`, the thresholds, `for` and the annotations stay as they are, so the alerts keep their labels. Seen on a v1.6.5 cluster while a Kubernetes cluster on a tenant etcd was created and deleted a few minutes later. vmalert had the critical alert firing for `grpc_method="Watch"` on all three members with the value 100. At that point `rate(grpc_server_handled_total{grpc_code!="OK"}[10m])` was 0.63 for Watch with `Canceled`, and the only other non-OK codes were `FailedPrecondition` on `RoleGet` and `UserGet` at 0.0017 each, which is one call each. The same query an hour earlier returned nothing. The platform copy runs in the same vmalert over the same series, and none of the firing alerts came from it. Each alert also showed up seven times there, once per tenant copy of these rules, because the root tenant's vmalert loads the rules of every tenant namespace. This PR does not change that. `hack/etcd-grpc-alerts.bats` renders the chart's PrometheusRule with `helm template` and replays it through `vmalert-tool` v1.136.0 pinned by digest, the same image #4963 uses. `hack/testdata/etcd-grpc-alerts/unittest.yaml` has three cases with seven checks. Watch streams that end only with `Canceled` must not fire at 6, 12 and 30 minutes. `Range` calls failing with `Unavailable` at 25 percent must fire the critical rule at 7 minutes and both rules at 12, with the labels and the summary pinned. Both together must fire only for `Range`. Against the rules on main the suite fails 4 of the 7 checks, all of them on Watch alerts with the value 100. Dropping `Unavailable` from the list, or grouping by `job` and `instance` only, fails 3 checks each. ```console $ bats hack/etcd-grpc-alerts.bats 1..1 ok 1 tenant etcd failed gRPC request alerts pass their vmalert-tool cases ``` The same run on the rules from main prints `not ok 1` and the four failing checks. With Bats 1.14.0 at the commit CI pins, the suite passes together with `bats-strict-setup`, `bats-no-negated-assert`, `bats-no-exit-trap`, `bats-runner-coverage`, `bats-runner-contract`, `alert-severity-contract` and `image-pin-consistency`, 81 of 81. Like the migration suites it needs docker, and only the image pull touches the network. Helm 3.17.3 and 4.2.4 render the same rules. I have not run the new rules in vmalert on a live cluster. The rules file is the same on release-1.6 and release-1.7. The commit cherry-picks onto release-1.6 without conflicts, and that backport is a separate PR with one more commit that drops the `load test_helper` line, because release-1.6 runs `hack/*.bats` through `hack/cozytest.sh`. ### Screenshots Not a UI change. ### Downstream repositories A rule template in `packages/extra/etcd`, a Bats suite under `hack/` and its test data. No values, schema, README, image or `ApplicationDefinition` change, and the website does not mention these rules, so no trigger map entry applies. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(etcd): the tenant etcd alert etcdHighNumberOfFailedGRPCRequests now counts only the gRPC codes that the etcd mixin and the platform copy of the alert count, so the Canceled watch streams of a Kubernetes API server that stops no longer fire it on every etcd member. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Refined etcd gRPC failure alerts to target specific failure status codes, reducing alerts for non-failure responses while preserving existing thresholds and timing. - Added coverage for canceled watch streams, unavailable range requests, and combined failure scenarios, including warning and critical alert evaluations. - **Tests** - Added integration validation for rendering etcd alert rules and running them through the alert-testing tool, including failure handling when prerequisites are unavailable. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Backported-from: 432d02e
OKgRPC code is not the only one that is successful, this also capturedcontext canceled, NotFound, and other non-error requests codes.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.