-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
no-default-scrape-port
feature flag
#9523
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
83e226a
to
1da21f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach is breaking and would create new timeseries. I do not think we should change the actual labels, just the HTTP query.
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. |
Then again, it doesn't matter so much. |
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. |
It should be visible in the prometheus/scrape/manager_test.go Lines 410 to 412 in 1da21f9
|
4e3cebc
to
b0583bd
Compare
@roidelapluie friendly ping :) |
b0583bd
to
9fd20c5
Compare
if a rebase is done here, would it be possible to merge this PR ? @LeviHarrison @roidelapluie ? |
a16ea75
to
90a0315
Compare
Signed-off-by: Levi Harrison <[email protected]>
Signed-off-by: Levi Harrison <[email protected]>
Signed-off-by: Levi Harrison <[email protected]>
Signed-off-by: Levi Harrison <[email protected]>
Signed-off-by: Levi Harrison <[email protected]>
Signed-off-by: Levi Harrison <[email protected]>
Signed-off-by: Levi Harrison <[email protected]>
Signed-off-by: Levi Harrison <[email protected]>
Signed-off-by: Levi Harrison <[email protected]>
c787048
to
76d370b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
docs/feature_flags.md
Outdated
When enabled, the default port for HTTP or HTTPS won't be added a target's address if no port is specified. | ||
In addition, if a default port (`:80` or `:443`) has already been attached, it will be removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out. I took a shot at writing a clearer and more comprehensive description.
896854a
to
76d370b
Compare
Signed-off-by: Levi Harrison <[email protected]>
// no-default-scrape-port flag is present, remove the port. | ||
switch port { | ||
case "80": | ||
if scheme == "http" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Line 411 in ef22afa
case "http", "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think scheme can be empty , but maybe it can with relabeling?
Thanks! |
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'm aware that Nginx automatically ignores the I have a hard time imagining scenarios where having |
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. |
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