no-default-scrape-port feature flag

[LeviHarrison]

This PR adds the no-default-scrape-port feature flag, which when enabled does not add the default HTTP or HTTPS port to the target's address.

Fixes #9507

[roidelapluie]

The issue is that SD's will add the port in general. And it often defaults to :80 and :443.

The logic should also include:

if feature flag:
   if scheme = http and address [-3] == :80 => address = address[:-3]
   if scheme = https and address [-4] == :443 => address = address[:-4]
endif

[roidelapluie]

The current approach is breaking and would create new timeseries. I do not think we should change the actual labels, just the HTTP query.

[LeviHarrison]

My worry then is that this process wouldn't be as opaque. You would think that the value of the address label is the exact address being used for the scrape. Users explicitly opt-in to this breaking change with the feature flag.

[LeviHarrison]

Then again, it doesn't matter so much.

[roidelapluie]

Actually, while you "opt-in" for this change, it would be handy to see the port added by service discovery in address. I think we should only be solving the HTTP-level issue.

[LeviHarrison]

It should be visible in the instance label

prometheus/scrape/manager_test.go

Lines 410 to 412 in 1da21f9

	res: labels.FromMap(map[string]string{
	model.AddressLabel: "1.2.3.4",
	model.InstanceLabel: "1.2.3.4:443",

.

[LeviHarrison]

@roidelapluie friendly ping :)

[Nexucis]

if a rebase is done here, would it be possible to merge this PR ? @LeviHarrison @roidelapluie ?

[roidelapluie]

LGTM

[bboreham]

s/won't be added a/won't be added to a/

Could you add some detail on where this takes effect?
I see the main change is in scrape.PopulateLabels - does it only impact labels?
Does it affect the address sent in the http request?
Who would have "already attached" a port?

[LeviHarrison]

Thanks for pointing that out. I took a shot at writing a clearer and more comprehensive description.

[LeviHarrison]

@roidelapluie should this accept only an explicit http scheme, whereas when adding the port above both an explicit http and a clear scheme are accepted? I'm leaning towards yes, but I'm unsure.

prometheus/scrape/target.go

Line 411 in ef22afa

case "http", "":

[roidelapluie]

I don't think scheme can be empty , but maybe it can with relabeling?

[roidelapluie]

Thanks!

[fjerhammer]

Sorry for reviving an old PR, but please consider making this the default. I have never seen any other HTTP(S) client with the current behaviour.
I spent a while investigating why Prometheus couldn't reach my scraper target, until I realized that I had to reconfigure my reverse proxy (relayd) to listen for matches on my.domain:443 in addition to just my.domain, even though it has been handling requests from a lot of obscure clients throughout the years without ever hitting this issue.

I'm aware that Nginx automatically ignores the :80 or :443 suffix of the Host header when matching requests received on these ports, but that isn't the norm, unfortunately.
RFC2616 states that a Host header "without any trailing port information implies the default port for the service requested", so it doesn't specifically forbid the inclusion of the default port numbers, but it's definitely not typical compared to other HTTP client software.

I have a hard time imagining scenarios where having no-default-scrape-port as the default would break anything, as opposed to solving potential issues for people with "weird" proxies like mine. Not to mention deployments switching from Nginx to something else (say, an organization upscaling to a hardware load balancer) that doesn't perform the same magic on the Host header, and suddenly seeing this issue out of nowhere.
Perhaps changing the default might cause issues when speaking directly to software using homegrown HTTP server implementions for exposing their metrics, relying on a specific Host header value using an exact match. But such servers would probably run on a non-default port anyway, in which case the port number would always be included in the Host request header.

[beorn7]

This should definitely be considered for Prometheus v3. (It would have been a breaking change and therefore could not have become the default with Prometheus v2.)

I have filed #13959 to make the call.