Add optional scheme support for push gateway urls #103
Conversation
| @@ -105,6 +106,8 @@ def write_to_textfile(path, registry): | |||
| 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. | |||
There was a problem hiding this comment.
"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.
|
Looks like tests failed on 2.6. My gut feeling is that |
|
And that feeling would be incorrect: https://docs.python.org/2.6/library/urlparse.html#urlparse.urlparse |
|
I also meant to followup with a question about the |
|
/metrics is what Prometheus scrapes, the pushgateway only supports /metric/ for incoming metrics. |
|
Oh wait, sorry. I understand what you mean now. You're right. |
|
Welp, 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 |
|
We should continue to support 2.6 |
|
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? |
| @@ -106,7 +106,13 @@ def write_to_textfile(path, registry): | |||
| 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. | |||
| `gateway` is a url. Scheme defaults to 'http' if none is provided | |||
| `job` is the name of the local routine pushing metrics | |||
There was a problem hiding this comment.
Job is the job label to use
|
Why would you need to remove the port number? |
| @@ -105,6 +106,14 @@ def write_to_textfile(path, registry): | |||
| 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 | |||
There was a problem hiding this comment.
Make it clear how much of the pgw url you need to provide
|
Python 2.6.9 doesn't correctly parse urls of the scheme |
|
I elected to just do a |
|
Thanks! |
We are using a push gateway behind an https reverse proxy.
push_gatewayand 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