Fix the Route Prefix bug and add ExternalURL Support (#190) #239

apghero · 2019-03-27T08:34:58Z

This fixes #190, I believe.

I've gotta figure out how to add in the --web.external-url with the same semantics as prometheus still. But, this should address the route-prefix issue as it stands right now.

Happy to address the externalURL in another PR entirely, in which case this wouldn't be work in progress anymore, and would be ready for review.

Pass in the prefix to the Static handler such that it can be removed from the Path looked up to serve files relative to the FileSystem's root. Signed-off-by: Andrew G10z <[email protected]>

Signed-off-by: Andrew G10z <[email protected]>

This works in conjunction with the `--web.route-prefix` flag to provide resources at the provided external URL, with the properly munged `route-prefix`. If an external URL is provided, but a `route-prefix` is not, this change takes the external URL's Path and uses that as the `route-prefix`. Includes a simple test checking for the presence of a known external-url in the body of the Status handler. Signed-off-by: Andrew G10z <[email protected]>

apghero · 2019-03-28T03:38:54Z

@beorn7: This should be ready to take a look at when you have a chance.

I did notice that the asset compilation doesn't work for repositories outside of the $GOPATH, which, since this repo is using modules shouldn't be required. I'll file another bug for that.

beorn7 · 2019-03-28T11:56:05Z

Many thanks, I'll have a look ASAP.

beorn7

As far as I can see, this should do the right thing (but I have a lot of trouble myself each time I try to wrap my mind around this prefix business).
Just a few comments about details.

beorn7 · 2019-04-01T21:46:08Z

handler/misc_test.go

@@ -0,0 +1,75 @@
+// Copyright 2014 The Prometheus Authors


New files should have the current year in the Copyright line. (Not that it matters a lot, it's just what we try to adhere to.)

beorn7 · 2019-04-01T21:46:23Z

handler/status_test.go

@@ -0,0 +1,52 @@
+// Copyright 2014 The Prometheus Authors


As above: New files should have the current year in the Copyright line. (Not that it matters a lot, it's just what we try to adhere to.)

beorn7 · 2019-04-01T21:46:57Z

main.go

@@ -49,6 +50,7 @@ func main() {

 		listenAddress       = app.Flag("web.listen-address", "Address to listen on for the web interface, API, and telemetry.").Default(":9091").String()
 		metricsPath         = app.Flag("web.telemetry-path", "Path under which to expose metrics.").Default("/metrics").String()
+		externalURL         = app.Flag("web.external-url", "The URL under which Prometheus is externally reachable.").Default("").URL()


s/Prometheus/the Pushgateway/

beorn7 · 2019-04-01T21:55:26Z

main.go

@@ -49,6 +50,7 @@ func main() {

 		listenAddress       = app.Flag("web.listen-address", "Address to listen on for the web interface, API, and telemetry.").Default(":9091").String()
 		metricsPath         = app.Flag("web.telemetry-path", "Path under which to expose metrics.").Default("/metrics").String()
+		externalURL         = app.Flag("web.external-url", "The URL under which Prometheus is externally reachable.").Default("").URL()
 		routePrefix         = app.Flag("web.route-prefix", "Prefix for the internal routes of web endpoints.").Default("").String()


The doc string should then also be adjusted to the one from Prometheus, i.e.:

"Prefix for the internal routes of web endpoints. Defaults to path of --web.external-url."

beorn7 · 2019-04-01T22:09:46Z

main.go

@@ -135,6 +135,25 @@ func handlePprof(w http.ResponseWriter, r *http.Request, p httprouter.Params) {
 	}
 }

+func computeRoutePrefix(prefix string, externalURL *url.URL) string {


The inline comments in this function are essentially just describing the (very simple) code and thus make it actually less readable. I guess the point you are trying to make is that this code implements the spec given by the flags. How about turning them into a doc comment before the function:

// computeRoutePrefix returns the effective route prefix based on the provided // flag values for --web.route-prefix and--web.external-url. With prefix empty, // the path of externalURL is used instead. A prefix "/" results in an empty // returned prefix. Any non-empty prefix is normalized to start, but not to end, // with "/".

beorn7 · 2019-04-01T22:21:21Z

handler/status.go

@@ -65,6 +67,14 @@ func Status(
 				"value": func(f float64) string {
 					return strconv.FormatFloat(f, 'f', -1, 64)
 				},
+				"join": func(base string, paths ...string) string {


Instead of passing in a whole new function, wouldn't it be easier to just pass in the pre-calculated complete string to prefix URLs in the template with? The function is always called in the same way in the template.

I can do that. The general purpose function is useful for future uses, but those uses don't currently exist, and are unlikely to be drastically different I'm guessing.

apghero · 2019-04-02T00:06:31Z

Oh boy. Some of these things are silly (copyright notice). I'll fix 'em up.

Signed-off-by: Andrew G10z <[email protected]>

apghero · 2019-04-02T03:09:48Z

OK, BaseURL is precomputed before template render.

beorn7 · 2019-04-02T21:07:27Z

Many thanks. Let's hope this really fixes the issue for good.

nodox · 2019-04-08T18:55:39Z

Have we stopped trying to release this fix? Would love this functionality but it seems the latest release was in December 2018.

beorn7 · 2019-04-08T22:00:49Z

A releases will happen soon. For this particular feature, I would really like to see it being tried out a bit by early adopters, who build from the tip of master. @nodox would be great if you volunteered.

nodox · 2019-04-09T15:40:05Z

Building from master might be too involved from me. However if you create a docker image with the changes in master you've got yourself a beta tester. You can add an alpha/beta tag or whatever tage makes sense. Right now I'm trying to make this work using the Prometheus Helm chart on a Kubernetes. @beorn7

sepich · 2019-04-09T18:50:35Z

Thank you,
at least now I've got it working with image prom/pushgateway:master,
args: --web.route-prefix=/push/,
and nginx location:

location /push {
    proxy_pass http://pushgateway:9091;
}

REST API also successfully receiving POSTs on host.domain.local/push/

When i've tried with --web.route-prefix=https://host.domain.local/push/ (same as for prometheus) - then all links to static resources became: /host.domain.local/push/script.js

beorn7 · 2019-04-10T10:19:17Z

Thanks @sepich . I'll have a look ASAP.

beorn7 · 2019-04-11T20:43:34Z

For me, both Prometheus and PGW panic if I set --web.route-prefix=https://host.domain.local/push/:

panic: wildcards must be named with a non-empty name in path '/https://host.domain.local/push/-/healthy'

I think rejecting it is the right thing as --web.route-prefix is just a path prefix, not a whole URL. We could make the error message nicer, of course.

@sepich did you manage to avoid the panic somehow?

apghero · 2019-04-11T20:49:26Z

@sepich have you tried --web.external-url=https://host.domain.local/push/ ? That should do the right thing as it takes the path of the external-url and puts it as the --web.route-prefix.

I agree with @beorn7 that the prefix should be better validated.

sepich · 2019-04-11T21:25:49Z

Ups, sorry for the typo.
I've meant --web.external-url=https://host.domain.local/push/ leads to:
<link rel="stylesheet" href="https:/host.domain.local/push/static/bootstrap-3.3.4-dist/css/bootstrap.min.css">
note single slash after https:,
which Chrome interpret as:
GET https://host.domain.local/host.domain.local/push/static/bootstrap-3.3.4-dist/css/bootstrap.min.css net::ERR_ABORTED 404

Yes, the --web.route-prefix=/push/ works fine.

beorn7 · 2019-04-11T21:56:57Z

I see. I'll try and fix.

beorn7 · 2019-04-11T22:46:48Z

#244 should fix it.

apghero added 2 commits March 27, 2019 01:35

Addresses route-prefix bug described in prometheus#190

e61ed49

Pass in the prefix to the Static handler such that it can be removed from the Path looked up to serve files relative to the FileSystem's root. Signed-off-by: Andrew G10z <[email protected]>

Add some tests for modified Static.

ebf02f6

Signed-off-by: Andrew G10z <[email protected]>

apghero force-pushed the apghero/bugfix/190/route-prefix-bug branch from df5afa7 to ebf02f6 Compare March 27, 2019 08:36

apghero added 2 commits March 27, 2019 01:37

Add License header to new test file.

e5fd860

Signed-off-by: Andrew G10z <[email protected]>

apghero changed the title ~~[WIP] Fix the Route Prefix bug and add ExternalURL Support (#190)~~ Fix the Route Prefix bug and add ExternalURL Support (#190) Mar 28, 2019

beorn7 requested changes Apr 1, 2019

View reviewed changes

apghero added 2 commits April 1, 2019 17:15

Update copyright dates.

5c60598

Signed-off-by: Andrew G10z <[email protected]>

Update documentation based on feedback.

0f0cc1f

Signed-off-by: Andrew G10z <[email protected]>

apghero force-pushed the apghero/bugfix/190/route-prefix-bug branch from 8b07fb8 to 0f0cc1f Compare April 2, 2019 00:16

Precompute BaseURL instead of passing join into template.

e747fea

Signed-off-by: Andrew G10z <[email protected]>

apghero force-pushed the apghero/bugfix/190/route-prefix-bug branch from ed25c35 to e747fea Compare April 2, 2019 03:09

beorn7 approved these changes Apr 2, 2019

View reviewed changes

beorn7 merged commit 6b81e91 into prometheus:master Apr 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the Route Prefix bug and add ExternalURL Support (#190) #239

Fix the Route Prefix bug and add ExternalURL Support (#190) #239

apghero commented Mar 27, 2019 •

edited

Loading

apghero commented Mar 28, 2019

beorn7 commented Mar 28, 2019

beorn7 left a comment

beorn7 Apr 1, 2019

beorn7 Apr 1, 2019

beorn7 Apr 1, 2019

beorn7 Apr 1, 2019

beorn7 Apr 1, 2019

beorn7 Apr 1, 2019

apghero Apr 2, 2019

apghero commented Apr 2, 2019

apghero commented Apr 2, 2019

beorn7 commented Apr 2, 2019

nodox commented Apr 8, 2019

beorn7 commented Apr 8, 2019

nodox commented Apr 9, 2019 •

edited

Loading

sepich commented Apr 9, 2019

beorn7 commented Apr 10, 2019

beorn7 commented Apr 11, 2019

apghero commented Apr 11, 2019

sepich commented Apr 11, 2019

beorn7 commented Apr 11, 2019

beorn7 commented Apr 11, 2019

Fix the Route Prefix bug and add ExternalURL Support (#190) #239

Fix the Route Prefix bug and add ExternalURL Support (#190) #239

Conversation

apghero commented Mar 27, 2019 • edited Loading

apghero commented Mar 28, 2019

beorn7 commented Mar 28, 2019

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 Apr 1, 2019

Choose a reason for hiding this comment

beorn7 Apr 1, 2019

Choose a reason for hiding this comment

beorn7 Apr 1, 2019

Choose a reason for hiding this comment

beorn7 Apr 1, 2019

Choose a reason for hiding this comment

beorn7 Apr 1, 2019

Choose a reason for hiding this comment

beorn7 Apr 1, 2019

Choose a reason for hiding this comment

apghero Apr 2, 2019

Choose a reason for hiding this comment

apghero commented Apr 2, 2019

apghero commented Apr 2, 2019

beorn7 commented Apr 2, 2019

nodox commented Apr 8, 2019

beorn7 commented Apr 8, 2019

nodox commented Apr 9, 2019 • edited Loading

sepich commented Apr 9, 2019

beorn7 commented Apr 10, 2019

beorn7 commented Apr 11, 2019

apghero commented Apr 11, 2019

sepich commented Apr 11, 2019

beorn7 commented Apr 11, 2019

beorn7 commented Apr 11, 2019

apghero commented Mar 27, 2019 •

edited

Loading

nodox commented Apr 9, 2019 •

edited

Loading