Repository: VictoriaMetrics/VictoriaMetrics
Pull request #6798 (2024-08-13 03:32:38)

vmagent: stop adding default port 80/433
### Describe Your Changes

Stop adding default port `80`/`443` for scrape URL without a port. See: https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6792

### Checklist

The following checks are **mandatory**:

- [x] My change adheres [VictoriaMetrics contributing guidelines](https://docs.victoriametrics.com/contributing/).


Haleygo: This is a breaking change, we should mention it in the changelog if we make it the default behavior.
Also, if the target is behind proxy like nginx which ignores `:80` or `:443` suffix, both `address:80` and `address` can work as scrape addresses. After apply this, the `instance` label value changes from `address:80` to `address`, which generates new timeseries.
jiekun: I've updated the CHANGELOG with "Update note" to mention the breaking change. 

> which generates new timeseries.

I noticed this issue while modifying unit tests. I think users may **not** be happy for generating new time series. I checked the Prometheus `no-default-scrape-port` flag, which also generates new time series, and there are some discussions on it: https://github.com/prometheus/prometheus/pull/9523#issuecomment-2059314966

Further discussion regarding these behaviors is welcome.

Patch:
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index 52f508b6a..4f7e0f06e 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -30,11 +30,14 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/).
 ## tip
 
 **Update note 1: The `external_labels` field in vmalert-tool [test file](https://docs.victoriametrics.com/vmalert-tool/#test-file-format) will be deprecated soon. Please use `-external.label` command-line flag instead, in the same way as vmalert uses it. This change is done for the sake of consistency between vmalert and vmalert-tool configuration. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6735).**
+**Update note 2: starting from this release, [vmagent](https://docs.victoriametrics.com/vmagent/) and [Single-node VictoriaMetrics](https://docs.victoriametrics.com/) will no longer add the default port 80/443 for scrape URLs without a specified port. 
+The value of `instance` label for those scrape targets will be changed from `<address>:<80|443>` to `<address>`, generating new time series. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6792) for details.**
 
 * FEATURE: add `/influx/health` health-check handler for Influx endpoints. This is needed as some clients use the health endpoint to determine if the server is healthy and ready for data ingestion. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6653) for the details.
 * FEATURE: [vmctl](https://docs.victoriametrics.com/vmctl/): add `--vm-backoff-retries`, `--vm-backoff-factor`, `--vm-backoff-min-duration` and `--vm-native-backoff-retries`, `--vm-native-backoff-factor`, `--vm-native-backoff-min-duration` command-line flags. These flags allow to change backoff policy config for import requests to VictoriaMetrics. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6622).
 * FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent/): allow overriding the `sample_limit` option at [scrape_configs](https://docs.victoriametrics.com/sd_configs/#scrape_configs) when a label `__sample_limit__` is specified for target. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6665). Thanks to @zoglam for the [pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6666).
 * FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent/): reduce memory usage when scraping targets with big response body. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6759).
+* FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent/) and [Single-node VictoriaMetrics](https://docs.victoriametrics.com/): stop adding default port 80/443 for scrape URLs without a port. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6792).
 * FEATURE: [vmalert](https://docs.victoriametrics.com/vmalert/): add command-line flag `-notifier.headers` to allow configuring additional headers for all requests sent to the corresponding `-notifier.url`.
 * FEATURE: [vmalert-tool](https://docs.victoriametrics.com/vmalert-tool/): add `-external.label` and `-external.url` command-line flags, in the same way as these flags are supported by vmalert. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6735).
 * FEATURE: [vmbackup](https://docs.victoriametrics.com/vmbackup/), [vmrestore](https://docs.victoriametrics.com/vmrestore/), [vmbackupmanager](https://docs.victoriametrics.com/vmbackupmanager/): use exponential backoff for retries when uploading or downloading data from S3. This should reduce the number of failed uploads and downloads when S3 is temporarily unavailable. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6732).
diff --git a/lib/promrelabel/scrape_url.go b/lib/promrelabel/scrape_url.go
index 55d7d5d5d..7533d7547 100644
--- a/lib/promrelabel/scrape_url.go
+++ b/lib/promrelabel/scrape_url.go
@@ -37,7 +37,6 @@ func GetScrapeURL(labels *promutils.Labels, extraParams map[string][]string) (st
 		metricsPath = address[n:]
 		address = address[:n]
 	}
-	address = addMissingPort(address, scheme == "https")
 
 	if !strings.HasPrefix(metricsPath, "/") {
 		metricsPath = "/" + metricsPath
@@ -90,25 +89,4 @@ func buildScrapeURL(scheme, address, metricsPath, optionalQuestion, paramsStr st
 	return s
 }
 
-func addMissingPort(addr string, isTLS bool) string {
-	if strings.Contains(addr, ":") {
-		return addr
-	}
-	if isTLS {
-		return concatTwoStrings(addr, ":443")
-	}
-	return concatTwoStrings(addr, ":80")
-}
-
-func concatTwoStrings(x, y string) string {
-	bb := bbPool.Get()
-	b := bb.B[:0]
-	b = append(b, x...)
-	b = append(b, y...)
-	s := bytesutil.InternBytes(b)
-	bb.B = b
-	bbPool.Put(bb)
-	return s
-}
-
 var bbPool bytesutil.ByteBufferPool


Test patch:
diff --git a/lib/promrelabel/scrape_url_test.go b/lib/promrelabel/scrape_url_test.go
index de8da3a32..7bdbdb5a1 100644
--- a/lib/promrelabel/scrape_url_test.go
+++ b/lib/promrelabel/scrape_url_test.go
@@ -24,31 +24,35 @@ func TestGetScrapeURL(t *testing.T) {
 	f(`{foo="bar"}`, "", "")
 
 	// __address__ without port
-	f(`{__address__="foo"}`, "http://foo:80/metrics", "foo:80")
+	f(`{__address__="foo"}`, "http://foo/metrics", "foo")
 
 	// __address__ with explicit port
 	f(`{__address__="foo:1234"}`, "http://foo:1234/metrics", "foo:1234")
 
 	// explicit __scheme__
-	f(`{__address__="foo",__scheme__="https"}`, "https://foo:443/metrics", "foo:443")
+	f(`{__address__="foo",__scheme__="https"}`, "https://foo/metrics", "foo")
 	f(`{__address__="foo:1234",__scheme__="https"}`, "https://foo:1234/metrics", "foo:1234")
 
 	// explicit __metrics_path__
-	f(`{__address__="foo",__metrics_path__="abc"}`, "http://foo:80/abc", "foo:80")
-	f(`{__address__="foo",__metrics_path__="/abc"}`, "http://foo:80/abc", "foo:80")
-	f(`{__address__="foo",__metrics_path__="/ab/c?d=ef&aa=bb"}`, "http://foo:80/ab/c?d=ef&aa=bb", "foo:80")
+	f(`{__address__="foo",__metrics_path__="abc"}`, "http://foo/abc", "foo")
+	f(`{__address__="foo",__metrics_path__="/abc"}`, "http://foo/abc", "foo")
+	f(`{__address__="foo",__metrics_path__="/ab/c?d=ef&aa=bb"}`, "http://foo/ab/c?d=ef&aa=bb", "foo")
 
 	// explitit __param_*
-	f(`{__address__="foo",__param_x="y"}`, "http://foo:80/metrics?x=y", "foo:80")
-	f(`{__address__="foo",__param_x="y",__param_y="aa"}`, "http://foo:80/metrics?x=y&y=aa", "foo:80")
-	f(`{__address__="foo",__param_x="y",__metrics_path__="?abc=de"}`, "http://foo:80/?abc=de&x=y", "foo:80")
-	f(`{__address__="foo",__param_abc="y",__metrics_path__="?abc=de"}`, "http://foo:80/?abc=de&abc=y", "foo:80")
+	f(`{__address__="foo",__param_x="y"}`, "http://foo/metrics?x=y", "foo")
+	f(`{__address__="foo",__param_x="y",__param_y="aa"}`, "http://foo/metrics?x=y&y=aa", "foo")
+	f(`{__address__="foo",__param_x="y",__metrics_path__="?abc=de"}`, "http://foo/?abc=de&x=y", "foo")
+	f(`{__address__="foo",__param_abc="y",__metrics_path__="?abc=de"}`, "http://foo/?abc=de&abc=y", "foo")
 
 	// __address__ with metrics path and/or scheme
-	f(`{__address__="foo/bar/baz?abc=de"}`, "http://foo:80/bar/baz?abc=de", "foo:80")
+	f(`{__address__="foo/bar/baz?abc=de"}`, "http://foo/bar/baz?abc=de", "foo")
 	f(`{__address__="foo:784/bar/baz?abc=de"}`, "http://foo:784/bar/baz?abc=de", "foo:784")
 	f(`{__address__="foo:784/bar/baz?abc=de",__param_xx="yy"}`, "http://foo:784/bar/baz?abc=de&xx=yy", "foo:784")
 	f(`{__address__="foo:784/bar/baz?abc=de",__param_xx="yy",__scheme__="https"}`, "https://foo:784/bar/baz?abc=de&xx=yy", "foo:784")
-	f(`{__address__="http://foo/bar/baz?abc=de",__param_xx="yy"}`, "http://foo:80/bar/baz?abc=de&xx=yy", "foo:80")
-	f(`{__address__="https://foo/bar/baz?abc=de",__param_xx="yy"}`, "https://foo:443/bar/baz?abc=de&xx=yy", "foo:443")
+	f(`{__address__="http://foo/bar/baz?abc=de",__param_xx="yy"}`, "http://foo/bar/baz?abc=de&xx=yy", "foo")
+	f(`{__address__="https://foo/bar/baz?abc=de",__param_xx="yy"}`, "https://foo/bar/baz?abc=de&xx=yy", "foo")
+
+	// __address__ with 80 or 443 as port
+	f(`{__address__="http://foo:80"}`, "http://foo:80/metrics", "foo:80")
+	f(`{__address__="https://foo:443"}`, "https://foo:443/metrics", "foo:443")
 }
diff --git a/lib/promscrape/config_test.go b/lib/promscrape/config_test.go
index 29930d380..a73c5b5fd 100644
--- a/lib/promscrape/config_test.go
+++ b/lib/promscrape/config_test.go
@@ -224,23 +224,23 @@ scrape_configs:
 	sws := cfg.getStaticScrapeWork()
 	swsExpected := []*ScrapeWork{
 		{
-			ScrapeURL:      "http://host1:80/metric/path1?x=y",
+			ScrapeURL:      "http://host1/metric/path1?x=y",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "host1:80",
+				"instance": "host1",
 				"job":      "abc",
 			}),
 			jobNameOriginal: "abc",
 		},
 		{
-			ScrapeURL:      "https://host2:443/metric/path2?x=y",
+			ScrapeURL:      "https://host2/metric/path2?x=y",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "host2:443",
+				"instance": "host2",
 				"job":      "abc",
 			}),
 			jobNameOriginal: "abc",
@@ -459,24 +459,24 @@ scrape_configs:
   - files: ["testdata/file_sd.json", "testdata/file_sd*.yml"]
 `, []*ScrapeWork{
 		{
-			ScrapeURL:      "http://host1:80/abc/de",
+			ScrapeURL:      "http://host1/abc/de",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "host1:80",
+				"instance": "host1",
 				"job":      "foo",
 				"qwe":      "rty",
 			}),
 			jobNameOriginal: "foo",
 		},
 		{
-			ScrapeURL:      "http://host2:80/abc/de",
+			ScrapeURL:      "http://host2/abc/de",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "host2:80",
+				"instance": "host2",
 				"job":      "foo",
 				"qwe":      "rty",
 			}),
@@ -690,12 +690,12 @@ scrape_configs:
   - targets: ["s"]
 `, []*ScrapeWork{
 		{
-			ScrapeURL:      "http://s:80/metrics",
+			ScrapeURL:      "http://s/metrics",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "s:80",
+				"instance": "s",
 				"job":      "aa",
 			}),
 			jobNameOriginal: "aa",
@@ -712,12 +712,12 @@ scrape_configs:
   - targets: ["s"]
 `, []*ScrapeWork{
 		{
-			ScrapeURL:      "http://s:80/metrics",
+			ScrapeURL:      "http://s/metrics",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "s:80",
+				"instance": "s",
 				"job":      "aa",
 			}),
 			jobNameOriginal: "aa",
@@ -734,12 +734,12 @@ scrape_configs:
   - targets: ["s"]
 `, []*ScrapeWork{
 		{
-			ScrapeURL:      "http://s:80/metrics",
+			ScrapeURL:      "http://s/metrics",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "s:80",
+				"instance": "s",
 				"job":      "aa",
 			}),
 			jobNameOriginal: "aa",
@@ -756,12 +756,12 @@ scrape_configs:
   - targets: ["s"]
 `, []*ScrapeWork{
 		{
-			ScrapeURL:      "http://s:80/metrics",
+			ScrapeURL:      "http://s/metrics",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "s:80",
+				"instance": "s",
 				"job":      "aa",
 			}),
 			jobNameOriginal: "aa",
@@ -845,7 +845,7 @@ scrape_configs:
   - targets: [foobar]
 `, []*ScrapeWork{
 		{
-			ScrapeURL:       "https://foo.bar:443/foo/bar?p=x%26y&p=%3D",
+			ScrapeURL:       "https://foo.bar/foo/bar?p=x%26y&p=%3D",
 			ScrapeInterval:  54 * time.Second,
 			ScrapeTimeout:   5 * time.Second,
 			MaxScrapeSize:   maxScrapeSize.N,
@@ -853,7 +853,7 @@ scrape_configs:
 			HonorTimestamps: true,
 			DenyRedirects:   true,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "foo.bar:443",
+				"instance": "foo.bar",
 				"job":      "foo",
 				"x":        "y",
 			}),
@@ -861,7 +861,7 @@ scrape_configs:
 			jobNameOriginal: "foo",
 		},
 		{
-			ScrapeURL:       "https://aaa:443/foo/bar?p=x%26y&p=%3D",
+			ScrapeURL:       "https://aaa/foo/bar?p=x%26y&p=%3D",
 			ScrapeInterval:  54 * time.Second,
 			ScrapeTimeout:   5 * time.Second,
 			MaxScrapeSize:   maxScrapeSize.N,
@@ -869,7 +869,7 @@ scrape_configs:
 			HonorTimestamps: true,
 			DenyRedirects:   true,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "aaa:443",
+				"instance": "aaa",
 				"job":      "foo",
 				"x":        "y",
 			}),
@@ -877,23 +877,23 @@ scrape_configs:
 			jobNameOriginal: "foo",
 		},
 		{
-			ScrapeURL:      "http://1.2.3.4:80/metrics",
+			ScrapeURL:      "http://1.2.3.4/metrics",
 			ScrapeInterval: 8 * time.Second,
 			ScrapeTimeout:  8 * time.Second,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "1.2.3.4:80",
+				"instance": "1.2.3.4",
 				"job":      "qwer",
 			}),
 			jobNameOriginal: "qwer",
 		},
 		{
-			ScrapeURL:      "http://foobar:80/metrics",
+			ScrapeURL:      "http://foobar/metrics",
 			ScrapeInterval: 8 * time.Second,
 			ScrapeTimeout:  8 * time.Second,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
-				"instance": "foobar:80",
+				"instance": "foobar",
 				"job":      "asdf",
 			}),
 			jobNameOriginal: "asdf",
@@ -1103,13 +1103,13 @@ scrape_configs:
       job: yyy
 `, []*ScrapeWork{
 		{
-			ScrapeURL:      "http://pp:80/metrics?a=c&a=xy",
+			ScrapeURL:      "http://pp/metrics?a=c&a=xy",
 			ScrapeInterval: defaultScrapeInterval,
 			ScrapeTimeout:  defaultScrapeTimeout,
 			MaxScrapeSize:  maxScrapeSize.N,
 			Labels: promutils.NewLabelsFromMap(map[string]string{
 				"foo":      "bar",
-				"instance": "pp:80",
+				"instance": "pp",
 				"job":      "yyy",
 			}),
 			ExternalLabels: promutils.NewLabelsFromMap(map[string]string{