Add optional scheme support for push gateway urls by thekuffs · Pull Request #103 · prometheus/client_python

thekuffs · 2016-09-28T22:23:35Z

We are using a push gateway behind an https reverse proxy. push_gateway and related functions previously assumed http and prepended the scheme no matter the circumstances. This patch checks to see if the user has specified a scheme. And, if not, it defaults to http.

I ran the tests on both python 2.7.10 and 3.5.2 and they pass just fine. So we've got backward-compatibility. But I didn't see a very obvious way to get an https server into the test.

Thanks @brian-brazil

brian-brazil · 2016-09-28T22:28:45Z

prometheus_client/exposition.py

 def push_to_gateway(gateway, job, registry, grouping_key=None, timeout=None):
    '''Push metrics to the given pushgateway.

+    `gateway` is a url, but will assume http if no other scheme is provided.


"scheme defaults to http if none is provided" would be a bit clearer.

If we're documenting one arg, they all should get documented. Exactly what bits of the URL are needed should be specified.

thekuffs · 2016-09-28T22:35:13Z

Looks like tests failed on 2.6. My gut feeling is that urlparse probably assumes http in the absence of a scheme on 2.6. It's '' on all others.

thekuffs · 2016-09-28T22:38:21Z

And that feeling would be incorrect: https://docs.python.org/2.6/library/urlparse.html#urlparse.urlparse

thekuffs · 2016-09-28T22:45:46Z

I also meant to followup with a question about the /metric/ path segment. That's configurable on the push gateway (https://github.com/prometheus/pushgateway/blob/master/main.go#L40). But the python client always assumes /metric. It doesn't affect me, but might be annoying to others in the future. It would be a bit difficult to make assumptions about the provided string given that RFC 1808 specifies that a scheme is required to interpret it as a netloc (https://github.com/prometheus/pushgateway/blob/master/main.go#L40)

brian-brazil · 2016-09-29T06:11:46Z

/metrics is what Prometheus scrapes, the pushgateway only supports /metric/ for incoming metrics.

thekuffs · 2016-09-29T15:34:37Z

Oh wait, sorry. I understand what you mean now. You're right.

thekuffs · 2016-09-30T21:15:51Z

Welp, urlparse is actually bugged in 2.6.9

(Pdb) gateway
u'127.0.0.1:59541'
(Pdb) urlparse(gateway)
ParseResult(scheme=u'127.0.0.1', netloc='', path=u'59541', params='', query='', fragment='')
(Pdb) from urlparse import urlsplit
(Pdb) urlsplit(gateway)
SplitResult(scheme=u'127.0.0.1', netloc='', path=u'59541', query='', fragment='')

Looking at this bug https://bugs.python.org/issue754016 , it's been broken since Python 2.3 at least. Someone submitted a patch for some release in the meantime that wasn't accepted. And then it didn't actually get fixed until 2.7.

How would you like to handle this? I can do a cheap if '//' not in gateway: ... or I guess drop 2.6 support?

brian-brazil · 2016-09-30T21:24:49Z

We should continue to support 2.6

thekuffs · 2016-09-30T21:26:58Z

I guess this is bettter

if not (gateway.startswith('http://') or gateway.startswith('https://')):

I feel like regex is probably a little too much to bring in to solve this.

I could also split the port number off and reattach it after urlparse?

brian-brazil · 2016-09-30T21:31:04Z

prometheus_client/exposition.py


-    `gateway` is a url, but will assume http if no other scheme is provided.
+    `gateway` is a url. Scheme defaults to 'http' if none is provided
+    `job` is the name of the local routine pushing metrics


Job is the job label to use

brian-brazil · 2016-09-30T21:31:31Z

Why would you need to remove the port number?

brian-brazil · 2016-09-30T21:31:55Z

prometheus_client/exposition.py

 def push_to_gateway(gateway, job, registry, grouping_key=None, timeout=None):
    '''Push metrics to the given pushgateway.

+    `gateway` is a url. Scheme defaults to 'http' if none is provided


Make it clear how much of the pgw url you need to provide

thekuffs · 2016-09-30T21:34:20Z

Python 2.6.9 doesn't correctly parse urls of the scheme 127.0.0.1:23523, which is why it got tripped up in the tests. The issue is fixed in all other pythons. So I could test and split on ':' and reattach it after the call to urlparse.

thekuffs · 2016-09-30T22:18:01Z

I elected to just do a gateway.startswith() test. It solves the problem.

brian-brazil · 2016-10-01T08:06:30Z

Thanks!

Add optional scheme support for gateway urls

5ecb53c

brian-brazil reviewed Sep 28, 2016

View reviewed changes

Add requested documentation updates

6ab8733

brian-brazil reviewed Sep 30, 2016

View reviewed changes

Update documentation. Resolve python 2.6.9 bug

edf8fe9

Remove unneeded imports

92dbddc

brian-brazil merged commit 4197142 into prometheus:master Oct 1, 2016

thekuffs deleted the add_https_url_support branch October 3, 2016 14:19

Conversation

thekuffs commented Sep 28, 2016

Uh oh!

brian-brazil Sep 28, 2016

Choose a reason for hiding this comment

Uh oh!

thekuffs commented Sep 28, 2016

Uh oh!

thekuffs commented Sep 28, 2016

Uh oh!

thekuffs commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-brazil commented Sep 29, 2016

Uh oh!

thekuffs commented Sep 29, 2016

Uh oh!

thekuffs commented Sep 30, 2016

Uh oh!

brian-brazil commented Sep 30, 2016

Uh oh!

thekuffs commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-brazil Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

brian-brazil commented Sep 30, 2016

Uh oh!

brian-brazil Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

thekuffs commented Sep 30, 2016

Uh oh!

thekuffs commented Sep 30, 2016

Uh oh!

brian-brazil commented Oct 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thekuffs commented Sep 28, 2016 •

edited

Loading

thekuffs commented Sep 30, 2016 •

edited

Loading