vmalert: follow-up after d4ac4b7813 (#4659)

Signed-off-by: hagen1778 <roman@victoriametrics.com>

app/vmalert/Makefile

@@ -77,9 +77,8 @@ test-vmalert:
 
 run-vmalert: vmalert
 	./bin/vmalert -rule=app/vmalert/config/testdata/rules/rules2-good.rules \
-		-datasource.url=http://localhost:8428 \
-		-notifier.url=http://localhost:9093 \
-		-notifier.url=http://127.0.0.1:9093 \
+		-datasource.url=http://demo.robustperception.io:9090 \
+		-notifier.blackhole \
 		-remoteWrite.url=http://localhost:8428 \
 		-remoteRead.url=http://localhost:8428 \
 		-external.label=cluster=east-1 \

app/vmalert/README.md

@@ -1089,7 +1089,7 @@ The shortlist of configuration flags is the following:
      Prometheus Alertmanager URL, e.g. http://127.0.0.1:9093. List all Alertmanager URLs if it runs in the cluster mode to ensure high availability.
      Supports an array of values separated by comma or specified via multiple flags.
   -notifier.blackhole bool
-       Whether to blackhole alerting notifications. Enable this flag if you want vmalert to evaluate alerting rules without sending any notifications to external receivers (eg. alertmanager). `-notifier.url`, `-notifier.config` and `-notifier.blackhole` are mutually exclusive.
+     Whether to blackhole alerting notifications. Enable this flag if you want vmalert to evaluate alerting rules without sending any notifications to external receivers (eg. alertmanager). `-notifier.url`, `-notifier.config` and `-notifier.blackhole` are mutually exclusive.
   -pprofAuthKey string
      Auth key for /debug/pprof/* endpoints. It must be passed via authKey query arg. It overrides httpAuth.* settings
   -promscrape.consul.waitTime duration

app/vmalert/notifier/init.go

@@ -19,7 +19,9 @@ var (
 
 	addrs = flagutil.NewArrayString("notifier.url", "Prometheus Alertmanager URL, e.g. http://127.0.0.1:9093. "+
 		"List all Alertmanager URLs if it runs in the cluster mode to ensure high availability.")
-	blackHole = flag.Bool("notifier.blackhole", false, "Don't send any notifications to anywhere. -notifier.url and -notifier.blackhole and -notifier.config are mutually exclusive")
+	blackHole = flag.Bool("notifier.blackhole", false, "Whether to blackhole alerting notifications. "+
+		"Enable this flag if you want vmalert to evaluate alerting rules without sending any notifications to external receivers (eg. alertmanager). "+
+		"`-notifier.url`, `-notifier.config` and `-notifier.blackhole` are mutually exclusive.")
 
 	basicAuthUsername     = flagutil.NewArrayString("notifier.basicAuth.username", "Optional basic auth username for -notifier.url")
 	basicAuthPassword     = flagutil.NewArrayString("notifier.basicAuth.password", "Optional basic auth password for -notifier.url")
@@ -97,7 +99,7 @@ func Init(gen AlertURLGenerator, extLabels map[string]string, extURL string) (fu
 		}
 
 		staticNotifiersFn = func() []Notifier {
-			return []Notifier{NewBlackHoleNotifier()}
+			return []Notifier{newBlackHoleNotifier()}
 		}
 		return staticNotifiersFn, nil
 	}

app/vmalert/notifier/init_test.go

@@ -37,7 +37,33 @@ func TestInit(t *testing.T) {
 	}
 }
 
-func TestInitBlackHole(t *testing.T) {
+func TestInitNegative(t *testing.T) {
+	oldConfigPath := *configPath
+	oldAddrs := *addrs
+	oldBlackHole := *blackHole
+
+	defer func() {
+		*configPath = oldConfigPath
+		*addrs = oldAddrs
+		*blackHole = oldBlackHole
+	}()
+
+	f := func(path, addr string, bh bool) {
+		*configPath = path
+		*addrs = flagutil.ArrayString{addr}
+		*blackHole = bh
+		if _, err := Init(nil, nil, ""); err == nil {
+			t.Fatalf("expected to get error; got nil instead")
+		}
+	}
+
+	// *configPath, *addrs and *blackhole are mutually exclusive
+	f("/dummy/path", "127.0.0.1", false)
+	f("/dummy/path", "", true)
+	f("", "127.0.0.1", true)
+}
+
+func TestBlackHole(t *testing.T) {
 	oldBlackHole := *blackHole
 	defer func() { *blackHole = oldBlackHole }()
 
@@ -50,7 +76,7 @@ func TestInitBlackHole(t *testing.T) {
 
 	nfs := fn()
 	if len(nfs) != 1 {
-		t.Fatalf("expected to get 1 notifiers; got %d", len(nfs))
+		t.Fatalf("expected to get 1 notifier; got %d", len(nfs))
 	}
 
 	targets := GetTargets()
@@ -65,60 +91,3 @@ func TestInitBlackHole(t *testing.T) {
 		t.Fatalf("expected to get \"blackhole\"; got %q instead", nf1.Addr())
 	}
 }
-
-func TestInitBlackHoleWithNotifierUrl(t *testing.T) {
-	oldAddrs := *addrs
-	oldBlackHole := *blackHole
-	defer func() {
-		*addrs = oldAddrs
-		*blackHole = oldBlackHole
-	}()
-
-	*addrs = flagutil.ArrayString{"127.0.0.1", "127.0.0.2"}
-	*blackHole = true
-
-	_, err := Init(nil, nil, "")
-	if err == nil {
-		t.Fatalf("Expect Init to return error; instead got no error")
-	}
-
-}
-
-func TestInitBlackHoleWithNotifierConfig(t *testing.T) {
-	oldConfigPath := *configPath
-	oldBlackHole := *blackHole
-	defer func() {
-		*configPath = oldConfigPath
-		*blackHole = oldBlackHole
-	}()
-
-	*configPath = "/dummy/path"
-	*blackHole = true
-
-	fn, err := Init(nil, nil, "")
-	if err == nil {
-		t.Fatalf("Expect Init to return error; instead got no error")
-	}
-
-	if fn != nil {
-		t.Fatalf("expected no notifiers to be returned;but got %v instead", fn())
-	}
-}
-
-func TestInitWithNotifierConfigAndAddr(t *testing.T) {
-	oldConfigPath := *configPath
-	oldAddrs := *addrs
-
-	defer func() {
-		*configPath = oldConfigPath
-		*addrs = oldAddrs
-	}()
-
-	*addrs = flagutil.ArrayString{"127.0.0.1", "127.0.0.2"}
-	*configPath = "/dummy/path"
-
-	_, err := Init(nil, nil, "")
-	if err == nil {
-		t.Fatalf("Expect Init to return error; instead got no error")
-	}
-}

app/vmalert/notifier/notifier_blackhole.go

@@ -2,33 +2,34 @@ package notifier
 
 import "context"
 
-// BlackHoleNotifier is used when no notifications needs to be sent
-type BlackHoleNotifier struct {
+// blackHoleNotifier is a Notifier stub, used when no notifications need
+// to be sent.
+type blackHoleNotifier struct {
 	addr    string
 	metrics *metrics
 }
 
-// Send will not send any notifications. Only increase the alerts sent number.
-func (bh *BlackHoleNotifier) Send(_ context.Context, alerts []Alert, _ map[string]string) error { //nolint:revive
+// Send will send no notifications, but increase the metric.
+func (bh *blackHoleNotifier) Send(_ context.Context, alerts []Alert, _ map[string]string) error { //nolint:revive
 	bh.metrics.alertsSent.Add(len(alerts))
 	return nil
 }
 
 // Addr of black hole notifier
-func (bh BlackHoleNotifier) Addr() string {
+func (bh blackHoleNotifier) Addr() string {
 	return bh.addr
 }
 
 // Close unregister the metrics
-func (bh *BlackHoleNotifier) Close() {
+func (bh *blackHoleNotifier) Close() {
 	bh.metrics.alertsSent.Unregister()
 	bh.metrics.alertsSendErrors.Unregister()
 }
 
-// NewBlackHoleNotifier Create a new BlackHoleNotifier
-func NewBlackHoleNotifier() *BlackHoleNotifier {
+// newBlackHoleNotifier creates a new blackHoleNotifier
+func newBlackHoleNotifier() *blackHoleNotifier {
 	address := "blackhole"
-	return &BlackHoleNotifier{
+	return &blackHoleNotifier{
 		addr:    address,
 		metrics: newMetrics(address),
 	}

app/vmalert/notifier/notifier_blackhole_test.go

@@ -9,8 +9,7 @@ import (
 )
 
 func TestBlackHoleNotifier_Send(t *testing.T) {
-	bh := NewBlackHoleNotifier()
-
+	bh := newBlackHoleNotifier()
 	if err := bh.Send(context.Background(), []Alert{{
 		GroupID:     0,
 		Name:        "alert0",
@@ -20,16 +19,15 @@ func TestBlackHoleNotifier_Send(t *testing.T) {
 	}}, nil); err != nil {
 		t.Errorf("unexpected error %s", err)
 	}
-	alertCount := bh.metrics.alertsSent.Get()
 
+	alertCount := bh.metrics.alertsSent.Get()
 	if alertCount != 1 {
 		t.Errorf("expect value 1; instead got %d", alertCount)
 	}
 }
 
 func TestBlackHoleNotifier_Close(t *testing.T) {
-	bh := NewBlackHoleNotifier()
-
+	bh := newBlackHoleNotifier()
 	if err := bh.Send(context.Background(), []Alert{{
 		GroupID:     0,
 		Name:        "alert0",
@@ -48,6 +46,5 @@ func TestBlackHoleNotifier_Close(t *testing.T) {
 		if name == alertMetricName {
 			t.Errorf("Metric name should have unregistered.But still present")
 		}
-
 	}
 }

docs/CHANGELOG.md

@@ -43,6 +43,7 @@ The following `tip` changes can be tested by building VictoriaMetrics components
 * FEATURE: [vmalert](https://docs.victoriametrics.com/vmalert.html): expose `vmalert_remotewrite_send_duration_seconds_total` counter, which can be used for determining high saturation of every connection to remote storage with an alerting query `sum(rate(vmalert_remotewrite_send_duration_seconds_total[5m])) by(job, instance) > 0.9 * max(vmalert_remotewrite_concurrency) by(job, instance)`. This query triggers when a connection is saturated by more than 90%. This usually means that `-remoteWrite.concurrency` command-line flag must be increased in order to increase the number of concurrent writings into remote endpoint. See [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4516).
 * FEATUTE: [vmalert](https://docs.victoriametrics.com/vmalert.html): display the error message received during unsuccessful config reload in vmalert's UI. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4076) for details.
 * FEATUTE: [vmalert](https://docs.victoriametrics.com/vmalert.html): allow disabling of `step` param attached to [instant queries](https://docs.victoriametrics.com/keyConcepts.html#instant-query). This might be useful for using vmalert with datasources that to not support this param, unlike VictoriaMetrics. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4573) for details.
+* FEATUTE: [vmalert](https://docs.victoriametrics.com/vmalert.html): support option for "balckholing" alerting notifications if `-notifier.blackhole` cmd-line flag is set. Enable this flag if you want vmalert to evaluate alerting rules without sending any notifications to external receivers (eg. alertmanager). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4122) for details. Thanks to @venkatbvc for [the pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4639).
 * FEATURE: [vmauth](https://docs.victoriametrics.com/vmauth.html): expose `vmauth_user_request_duration_seconds` and `vmauth_unauthorized_user_request_duration_seconds` summary metrics for measuring requests latency per user.
 * FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup.html): show backup progress percentage in log during backup uploading. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4460).
 * FEATURE: [vmrestore](https://docs.victoriametrics.com/vmrestore.html): show restoring progress percentage in log during backup downloading. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4460).

docs/vmalert.md

@@ -1099,6 +1099,8 @@ The shortlist of configuration flags is the following:
   -notifier.url array
      Prometheus Alertmanager URL, e.g. http://127.0.0.1:9093. List all Alertmanager URLs if it runs in the cluster mode to ensure high availability.
      Supports an array of values separated by comma or specified via multiple flags.
+  -notifier.blackhole bool
+     Whether to blackhole alerting notifications. Enable this flag if you want vmalert to evaluate alerting rules without sending any notifications to external receivers (eg. alertmanager). `-notifier.url`, `-notifier.config` and `-notifier.blackhole` are mutually exclusive.
   -pprofAuthKey string
      Auth key for /debug/pprof/* endpoints. It must be passed via authKey query arg. It overrides httpAuth.* settings
   -promscrape.consul.waitTime duration