Repository: VictoriaMetrics/VictoriaMetrics
Pull request #3452 (2022-12-06 10:16:09)

vmalert: correctly return error for RW failures
By mistake, in 0989649ad041d5984f83fac6e2c76dd3b92af19a the error for remote write failures wasn't returned to user.
This change fixes it.

Signed-off-by: hagen1778 <roman@victoriametrics.com>

codecov[bot]: # [Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/3452?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) Report
Base: **57.71**% // Head: **57.75**% // Increases project coverage by **`+0.04%`** :tada:
> Coverage data is based on head [(`c4b11c5`)](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/3452?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) compared to base [(`0675865`)](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/commit/06758650bfd77a323daea4009666cac1af9f6262?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).
> Patch coverage: 66.66% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #3452      +/-   ##
==========================================
+ Coverage   57.71%   57.75%   +0.04%     
==========================================
  Files         287      287              
  Lines       49344    49350       +6     
==========================================
+ Hits        28478    28502      +24     
+ Misses      19035    19022      -13     
+ Partials     1831     1826       -5     
```


| [Impacted Files](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/3452?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) | Coverage Δ | |
|---|---|---|
| [app/vmalert/group.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/3452/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-YXBwL3ZtYWxlcnQvZ3JvdXAuZ28=) | `79.05% <66.66%> (+1.20%)` | :arrow_up: |
| [lib/storage/storage.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/3452/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-bGliL3N0b3JhZ2Uvc3RvcmFnZS5nbw==) | `70.66% <0.00%> (+0.11%)` | :arrow_up: |
| [app/vmselect/promql/aggr\_incremental.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/3452/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-YXBwL3Ztc2VsZWN0L3Byb21xbC9hZ2dyX2luY3JlbWVudGFsLmdv) | `87.68% <0.00%> (+2.40%)` | :arrow_up: |
| [app/vmalert/remotewrite/remotewrite.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/3452/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-YXBwL3ZtYWxlcnQvcmVtb3Rld3JpdGUvcmVtb3Rld3JpdGUuZ28=) | `65.75% <0.00%> (+3.42%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/3452?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).


Patch:
diff --git a/app/vmalert/group.go b/app/vmalert/group.go
index 7408d54a5..6e55ba25f 100644
--- a/app/vmalert/group.go
+++ b/app/vmalert/group.go
@@ -421,20 +421,26 @@ func (e *executor) exec(ctx context.Context, rule Rule, ts time.Time, resolveDur
 		return fmt.Errorf("rule %q: failed to execute: %w", rule, err)
 	}
 
-	errGr := new(utils.ErrGroup)
 	if e.rw != nil {
-		pushToRW := func(tss []prompbmarshal.TimeSeries) {
+		pushToRW := func(tss []prompbmarshal.TimeSeries) error {
+			var lastErr error
 			for _, ts := range tss {
 				remoteWriteTotal.Inc()
 				if err := e.rw.Push(ts); err != nil {
 					remoteWriteErrors.Inc()
-					errGr.Add(fmt.Errorf("rule %q: remote write failure: %w", rule, err))
+					lastErr = fmt.Errorf("rule %q: remote write failure: %w", rule, err)
 				}
 			}
+			return lastErr
+		}
+		if err := pushToRW(tss); err != nil {
+			return err
 		}
-		pushToRW(tss)
+
 		staleSeries := e.getStaleSeries(rule, tss, ts)
-		pushToRW(staleSeries)
+		if err := pushToRW(staleSeries); err != nil {
+			return err
+		}
 	}
 
 	ar, ok := rule.(*AlertingRule)
@@ -448,6 +454,7 @@ func (e *executor) exec(ctx context.Context, rule Rule, ts time.Time, resolveDur
 	}
 
 	wg := sync.WaitGroup{}
+	errGr := new(utils.ErrGroup)
 	for _, nt := range e.notifiers() {
 		wg.Add(1)
 		go func(nt notifier.Notifier) {
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index d73dc154a..899251f63 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -58,6 +58,7 @@ The following tip changes can be tested by building VictoriaMetrics components f
 
 * BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert.html): properly pass HTTP headers during the alert state restore procedure. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3418).
 * BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert.html): properly specify rule evaluation step during the [replay mode](https://docs.victoriametrics.com/vmalert.html#rules-backfilling). The `step` value was previously overriden by `-datasource.queryStep` command-line flag.
+* BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert.html): properly return the error message from remote-write failures. Before, error was ignored and only `vmalert_remotewrite_errors_total` was incremented.
 * BUGFIX: [vmui](https://docs.victoriametrics.com/#vmui): properly put multi-line queries in the url, so it could be copy-n-pasted and opened without issues in a new browser tab. Previously the url for multi-line query couldn't be opened. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3444).
 * BUGFIX: [vmui](https://docs.victoriametrics.com/#vmui): correctly handle `up` and `down` keypresses when editing multi-line queries. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3445).
 


Test patch:
diff --git a/app/vmalert/group_test.go b/app/vmalert/group_test.go
index 5c59e5c3d..c4f2d2a82 100644
--- a/app/vmalert/group_test.go
+++ b/app/vmalert/group_test.go
@@ -3,8 +3,6 @@ package main
 import (
 	"context"
 	"fmt"
-	"github.com/VictoriaMetrics/VictoriaMetrics/lib/decimal"
-	"github.com/VictoriaMetrics/VictoriaMetrics/lib/prompbmarshal"
 	"reflect"
 	"sort"
 	"testing"
@@ -12,6 +10,9 @@ import (
 
 	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmalert/config"
 	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmalert/notifier"
+	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmalert/remotewrite"
+	"github.com/VictoriaMetrics/VictoriaMetrics/lib/decimal"
+	"github.com/VictoriaMetrics/VictoriaMetrics/lib/prompbmarshal"
 	"github.com/VictoriaMetrics/VictoriaMetrics/lib/promutils"
 )
 
@@ -452,3 +453,24 @@ func TestFaultyNotifier(t *testing.T) {
 	}
 	t.Fatalf("alive notifier didn't receive notification by %v", deadline)
 }
+
+func TestFaultyRW(t *testing.T) {
+	fq := &fakeQuerier{}
+	fq.add(metricWithValueAndLabels(t, 1, "__name__", "foo", "job", "bar"))
+
+	r := &RecordingRule{
+		Name:  "test",
+		state: newRuleState(),
+		q:     fq,
+	}
+
+	e := &executor{
+		rw:                       &remotewrite.Client{},
+		previouslySentSeriesToRW: make(map[uint64]map[string][]prompbmarshal.Label),
+	}
+
+	err := e.exec(context.Background(), r, time.Now(), 0, 10)
+	if err == nil {
+		t.Fatalf("expected to get an error from faulty RW client, got nil instead")
+	}
+}