Repository: VictoriaMetrics/VictoriaMetrics
Pull request #4851 (2023-08-17 12:52:40)

lib/protoparser: handle unexpected EOF error when parsing lines in prometheus exposition format
This PR should fix the problem described in this feature request: https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4817

Previously only `io.EOF` was handled, and `io.ErrUnexpectedEOF` was ignored, but it may happen if the client interrupts the connection.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4851?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) Report
Patch coverage: **`100.00%`** and project coverage change: **`-0.02%`** :warning:
> Comparison is base [(`1e1a30e`)](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/commit/1e1a30ed7f7a42498968332162ae65d63a30e298?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) 60.60% compared to head [(`b0668e7`)](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4851?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) 60.58%.
> Report is 3 commits behind head on master.

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


```diff
@@            Coverage Diff             @@
##           master    #4851      +/-   ##
==========================================
- Coverage   60.60%   60.58%   -0.02%     
==========================================
  Files         384      384              
  Lines       72264    72260       -4     
==========================================
- Hits        43794    43780      -14     
- Misses      26074    26084      +10     
  Partials     2396     2396              
```


| [Files Changed](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4851?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics) | Coverage Δ | |
|---|---|---|
| [lib/protoparser/common/lines\_reader.go](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4851?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics#diff-bGliL3Byb3RvcGFyc2VyL2NvbW1vbi9saW5lc19yZWFkZXIuZ28=) | `94.64% <100.00%> (ø)` | |

... and [11 files with indirect coverage changes](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4851/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/4851?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VictoriaMetrics).   
:loudspeaker: Have feedback on the report? [Share it here](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/README.md b/README.md
index f792d1473..aebd03b8f 100644
--- a/README.md
+++ b/README.md
@@ -173,7 +173,8 @@ VictoriaMetrics is developed at a fast pace, so it is recommended periodically c
 
 ### Environment variables
 
-All the VictoriaMetrics components allow referring environment variables in command-line flags via `%{ENV_VAR}` syntax.
+All the VictoriaMetrics components allow referring environment variables in `yaml` configuration files (such as `-promscrape.config`)
+and in command-line flags via `%{ENV_VAR}` syntax.
 For example, `-metricsAuthKey=%{METRICS_AUTH_KEY}` is automatically expanded to `-metricsAuthKey=top-secret`
 if `METRICS_AUTH_KEY=top-secret` environment variable exists at VictoriaMetrics startup.
 This expansion is performed by VictoriaMetrics itself.
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index 7e89effa7..9703d8593 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -25,9 +25,10 @@ The following `tip` changes can be tested by building VictoriaMetrics components
 ## tip
 
 * FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup.html): add support for server-side copy of existing backups. See [these docs](https://docs.victoriametrics.com/vmbackup.html#server-side-copy-of-the-existing-backup) for details.
+* FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent.html): properly handle `unexpected EOF` error when parsing metrics in Prometheus exposition format. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4817). 
 
 * BUGFIX: do not allow starting VictoriaMetrics components with improperly set boolean command-line flags in the form `-boolFlagName value`, since this leads to silent incomplete flags' parsing. This form should be replaced with `-boolFlagName=value`. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4845).
-* BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): according to [the docs](https://docs.victoriametrics.com/vmagent.html#adding-labels-to-metrics) labels from `-remoteWrite.label` cmd-line flag are now added to the sent metrics just before they are pushed to the `-remoteWrite.url` (after all relabeling, including stream aggregation relabeling). In addition, it allows adding labels for identifying vmagent instances when using streaming aggregation in vmagents [cluster mode](https://docs.victoriametrics.com/vmagent.html#scraping-big-number-of-targets). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4247) and [these docs](https://docs.victoriametrics.com/stream-aggregation.html#cluster-mode) for more details.
+* BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): properly set labels from `-remoteWrite.label` command-line flag just before sending samples to the configured `-remoteWrite.url` according to [these docs](https://docs.victoriametrics.com/vmagent.html#adding-labels-to-metrics). Previously these labels were incorrectly set before [the relabeling](https://docs.victoriametrics.com/vmagent.html#relabeling) configured via `-remoteWrite.urlRelabelConfigs` and [the stream aggregation](https://docs.victoriametrics.com/stream-aggregation.html) configured via `-remoteWrite.streamAggr.config`, so these labels could be lost or incorrectly transformed before sending the samples to remote storage. The fix allows using `-remoteWrite.label` for identifying `vmagent` instances in [cluster mode](https://docs.victoriametrics.com/vmagent.html#scraping-big-number-of-targets). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4247) and [these docs](https://docs.victoriametrics.com/stream-aggregation.html#cluster-mode) for more details.
 * BUGFIX: remove `DEBUG` logging when parsing `if` filters inside [relabeling rules](https://docs.victoriametrics.com/vmagent.html#relabeling-enhancements) and when parsing `match` filters inside [stream aggregation rules](https://docs.victoriametrics.com/stream-aggregation.html).
 * BUGFIX: properly replace `:` chars in label names with `_` when `-usePromCompatibleNaming` command-line flag is passed to `vmagent`, `vminsert` or single-node VictoriaMetrics. This addresses [this comment](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3113#issuecomment-1275077071).
 * BUGFIX: [Official Grafana dashboards for VictoriaMetrics](https://grafana.com/orgs/victoriametrics): fix display of ingested rows rate for `Samples ingested/s` and `Samples rate` panels for vmagent's dasbhoard. Previously, not all ingested protocols were accounted in these panels. An extra panel `Rows rate` was added to `Ingestion` section to display the split for rows ingested rate by protocol.
diff --git a/docs/README.md b/docs/README.md
index 448d59f56..34412c343 100644
--- a/docs/README.md
+++ b/docs/README.md
@@ -176,7 +176,8 @@ VictoriaMetrics is developed at a fast pace, so it is recommended periodically c
 
 ### Environment variables
 
-All the VictoriaMetrics components allow referring environment variables in command-line flags via `%{ENV_VAR}` syntax.
+All the VictoriaMetrics components allow referring environment variables in `yaml` configuration files (such as `-promscrape.config`)
+and in command-line flags via `%{ENV_VAR}` syntax.
 For example, `-metricsAuthKey=%{METRICS_AUTH_KEY}` is automatically expanded to `-metricsAuthKey=top-secret`
 if `METRICS_AUTH_KEY=top-secret` environment variable exists at VictoriaMetrics startup.
 This expansion is performed by VictoriaMetrics itself.
diff --git a/docs/Single-server-VictoriaMetrics.md b/docs/Single-server-VictoriaMetrics.md
index b3e54edf7..d26ed865d 100644
--- a/docs/Single-server-VictoriaMetrics.md
+++ b/docs/Single-server-VictoriaMetrics.md
@@ -184,7 +184,8 @@ VictoriaMetrics is developed at a fast pace, so it is recommended periodically c
 
 ### Environment variables
 
-All the VictoriaMetrics components allow referring environment variables in command-line flags via `%{ENV_VAR}` syntax.
+All the VictoriaMetrics components allow referring environment variables in `yaml` configuration files (such as `-promscrape.config`)
+and in command-line flags via `%{ENV_VAR}` syntax.
 For example, `-metricsAuthKey=%{METRICS_AUTH_KEY}` is automatically expanded to `-metricsAuthKey=top-secret`
 if `METRICS_AUTH_KEY=top-secret` environment variable exists at VictoriaMetrics startup.
 This expansion is performed by VictoriaMetrics itself.
diff --git a/docs/stream-aggregation.md b/docs/stream-aggregation.md
index eb01c9f64..80756a3f0 100644
--- a/docs/stream-aggregation.md
+++ b/docs/stream-aggregation.md
@@ -678,8 +678,12 @@ support the following approaches for hot reloading stream aggregation configs fr
 
 If you use [vmagent in cluster mode](https://docs.victoriametrics.com/vmagent.html#scraping-big-number-of-targets) for streaming aggregation
 (with `-promscrape.cluster.*` parameters or with `VMAgent.spec.shardCount > 1` for [vmoperator](https://docs.victoriametrics.com/operator))
-then be careful when aggregating metrics via `by`, `without` or modifying via`*_relabel_configs` parameters, as incorrect usage may result in duplicates and data collision. For example, if more than one vmagent calculates `increase` for metric `http_requests_total` with `by: [path]` directive, then the resulting time series written to the remote database will be indistinguishable, as there is no way to tell to which `instance` the aggregation belongs. The proper fix would be to add a unique aggregation dimension: `by: [instance, path]`. With this change, aggregates between `instances` won't collide.
-
-If adding a new aggregation dimension isn't feasible (due to cardinality reduction purposes), then it is worth at least differentiating by which vmagent the aggregation was performed. You can do it with `remoteWrite.label` [parameter in vmagent](https://docs.victoriametrics.com/vmagent.html#adding-labels-to-metrics).
-For example, for running in docker or k8s you can use `remoteWrite.label` with `POD_NAME` or `HOSTNAME` environment variable: `remoteWrite.label='vmagent=%{HOSTNAME}'`.
-See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4247#issue-1692894073) for details.
+then be careful when aggregating metrics via `by`, `without` or modifying via `*_relabel_configs` parameters, since incorrect usage
+may result in duplicates and data collision. For example, if more than one `vmagent` instance calculates `increase` for metric `http_requests_total`
+with `by: [path]` directive, then all the `vmagent` instances will aggregate samples to the same set of time series with different `path` labels.
+The proper fix would be to add an unique [`-remoteWrite.label`](https://docs.victoriametrics.com/vmagent.html#adding-labels-to-metrics) per each `vmagent`,
+so every `vmagent` aggregates data into distinct set of time series. These time series then can be aggregated later as needed during querying.
+
+For example, if `vmagent` instances run in Docker or Kubernetes, then you can refer `POD_NAME` or `HOSTNAME` environment variables
+as an unique label value per each `vmagent`: `-remoteWrite.label='vmagent=%{HOSTNAME}` . See [these docs](https://docs.victoriametrics.com/#environment-variables)
+on how to refer environment variables in VictoriaMetrics components.
diff --git a/lib/protoparser/common/lines_reader.go b/lib/protoparser/common/lines_reader.go
index 1549a2079..a1fe0d3c6 100644
--- a/lib/protoparser/common/lines_reader.go
+++ b/lib/protoparser/common/lines_reader.go
@@ -93,7 +93,7 @@ again:
 }
 
 func isEOFLikeError(err error) bool {
-	if errors.Is(err, io.EOF) {
+	if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
 		return true
 	}
 	s := err.Error()


Test patch:
diff --git a/lib/protoparser/common/lines_reader_test.go b/lib/protoparser/common/lines_reader_test.go
index a083322c1..e37ac720c 100644
--- a/lib/protoparser/common/lines_reader_test.go
+++ b/lib/protoparser/common/lines_reader_test.go
@@ -2,6 +2,7 @@ package common
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"reflect"
@@ -25,6 +26,20 @@ func TestReadLinesBlockFailure(t *testing.T) {
 		if _, _, err := ReadLinesBlock(fr, nil, nil); err == nil {
 			t.Fatalf("expecting non-nil error")
 		}
+
+		un := &unexpectedEOF{}
+		if _, _, err := ReadLinesBlock(un, nil, nil); err != nil {
+			if !errors.Is(err, io.EOF) {
+				t.Fatalf("get unexpected error, expecting io.EOF")
+			}
+		}
+
+		ef := eofErr{}
+		if _, _, err := ReadLinesBlock(ef, nil, nil); err != nil {
+			if !errors.Is(err, io.EOF) {
+				t.Fatalf("get unexpected error, expecting io.EOF")
+			}
+		}
 	}
 
 	// empty string
@@ -41,6 +56,18 @@ func (fr *failureReader) Read(p []byte) (int, error) {
 	return 0, fmt.Errorf("some error")
 }
 
+type unexpectedEOF struct{}
+
+func (un unexpectedEOF) Read(p []byte) (int, error) {
+	return 0, io.ErrUnexpectedEOF
+}
+
+type eofErr struct{}
+
+func (eo eofErr) Read(p []byte) (int, error) {
+	return 0, io.EOF
+}
+
 func TestReadLinesBlockMultiLinesSingleByteReader(t *testing.T) {
 	f := func(s string, linesExpected []string) {
 		t.Helper()