Skip to content

contrib/mixin/mixin.libsonnet: Adjust gRPC failed requests - #13127

Merged
hexfusion merged 1 commit into
etcd-io:mainfrom
lilic:fix-grpc-req-alert
Jun 23, 2021
Merged

hexfusion merged 1 commit into
etcd-io:mainfrom
lilic:fix-grpc-req-alert

Conversation

@lilic

@lilic lilic commented Jun 21, 2021

Copy link
Copy Markdown
Contributor

OK gRPC code is not the only one that is successful, this also captured
context canceled, NotFound, and other non-error requests codes.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

OK is not the only one that is allowed, this before also captured
context canceled, NotFound, and other non error requests.
@lilic

lilic commented Jun 21, 2021

Copy link
Copy Markdown
Contributor Author

@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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@serathius let me know what you think, happy to reverse the order if you think it still makes sense here, thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's ok as it doesn't look there will be any difference in cost of maintaining this list up to date.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1 on the 'Canceled' code

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same here, only 'Canceled' is giving the noise

@hexfusion hexfusion left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+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.

[1]grpc/grpc-go@c463038

@hexfusion
hexfusion merged commit c44d89f into etcd-io:main Jun 23, 2021
@lilic
lilic deleted the fix-grpc-req-alert branch June 23, 2021 16:37
svparamzin added a commit to svparamzin/helm-charts that referenced this pull request May 17, 2022
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]>
myasnikovdaniil pushed a commit to cozystack/cozystack that referenced this pull request Oct 23, 2026
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]>
myasnikovdaniil added a commit to cozystack/cozystack that referenced this pull request Oct 23, 2026
…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 -->
myasnikovdaniil pushed a commit to cozystack/cozystack that referenced this pull request Nov 10, 2026
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>
myasnikovdaniil added a commit to cozystack/cozystack that referenced this pull request Nov 10, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

7 participants