Adding web.external-url and web.route-prefix flags

[snebel29]

Hi,
First all, let's make it clear that this is my first contribution within the Prometheus ecosystem, but hopefully not the last one.

This PR aims to fix #475 making several assumptions and having different feelings on the way.

• Keep consistency with alertmanager and prometheus which seems to implement the same flags, I essentially copied the same logic adapting for the specifics of this component.
• I'm not forcing absolute url when --web.external-url is set, since all links are anyway relative reusing the original domain, I guess in other components more complex such as prometheus or alertmanager this might be necessary. I'm happy to change the behavior if forcing absolute path is what we want.

Since this component is so simple I've coded this PR using the naive approach, but I feel it probably makes sense to move the following function with its test case to https://github.com/prometheus/common since the same function is used in the alertmanager as well (In fact It was literally copied from there)

May be creating something like https://github.com/prometheus/common/tree/master/promlog even trying to get the flags packed there, then implement in both blackbox_exporter and alertmanger and take a look to see if those flags are implemented anywhere else, but I would appreciate feedback on this point before doing anything.

[brian-brazil]

I'm happy to change the behavior if forcing absolute path is what we want.

Let's go with consistency.

May be creating something like https://github.com/prometheus/common/tree/master/promlog even trying to get the flags packed there, then implement in both blackbox_exporter and alertmanger and take a look to see if those flags are implemented anywhere else, but I would appreciate feedback on this point before doing anything.

Probably best to do that with the TLS stuff when it's moved over to common. It's also in the pushgateway.

@SuperQ FYI

[brian-brazil]

We could use relative paths here, the blackbox exporter doesn't do anything fancy.

[snebel29]

Great, since they are already relative urls I'll keep the code in this area the same then

[snebel29]

Sorry for bothering again, but let's see if I understood correctly

Let's go with consistency.

The only places were we could be using the absolute url value coming from web.external-url are

• ServeMux pattern of Handle and HandleFunc functions: But this would be inconsistent with other projects from the ecosystem, and kind of weird
• The html code returned by / path pattern matching: We already agreed on using relative path there

It's therefore my understanding that we are already consistent and ok on this point.

Probably best to do that with the TLS stuff when it's moved over to common. It's also in the pushgateway.

I'm not sure what you mean by TLS in the context of alertmanager and pushgateway, I can't see any obvious TLS stuff in their main files to move to common on them.

In any case my understanding is that you would want to address moving extURL together with "the TLS stuff" in a different PR.

Is this PR then ok to be merged?

[brian-brazil]

The only places were we could be using the absolute url value coming from web.external-url are

I meant consistent in the checks it performs. Having the semantics different across different servers would be confusing.

I'm not sure what you mean by TLS in the context of alertmanager and pushgateway

It's in progress in the node exporter currently, and it'd make sense for all this code to be together at some future point.

[snebel29]

I meant consistent in the checks it performs. Having the semantics different across different servers would be confusing.

I agree, I took the logic from the alertmanager, therefore the same parameters across servers of the ecosystem should produce the same result

[snebel29]

I will double-check prometheus, pushgateway and alertmanager behaviour for those two flags, to ensure the semantics are exactly the same (as it was intended).

Here is a recently implemented example prometheus/pushgateway#190 which code I could reuse I guess, allowing to be moved on to common later on.

[brian-brazil]

The logic in https://github.com/prometheus/prometheus/blob/d98d4a9bf04217f5b36fd3248cbe88d3046e84e4/cmd/prometheus/main.go#L791 is a bit different

[snebel29]

@brian-brazil I reviewed prometheus behavior for --web.external-url and --web.route-prefix and I think I've matched it accordingly, note that alertmanager from where I originally "took" the logic is slightly different, see here so may be as an intermediary step towards leveraging common would be good to migrate the function in the alertmanager as well, at first glance the function signature is different and this check is not performed in prometheus.

From what I can see, only one behaviour will remain different between prometheus <> blackbox_exporter after merging this PR, currently blackbox_exporter matches any non registered path to the landing page, see here, while prometheus return 404s when a path wasn't explicitly registered, but this the case already before this PR.

This is hard to match due to using ServeMux in blackbox_exporter vs github.com/prometheus/common/route by leveraging julienschmidt/httprouter which matches exact patterns only.

I guess in the future, migrating over github.com/prometheus/common/route would make things easier if the intention is to behave 100% the same over time, also easier to maintain.

I hope this makes sense now, but please let me know otherwise.

Cheers

[snebel29]

How about that?

[brian-brazil]

Thanks!