Finish improvements to push client #234
Conversation
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
2 times, most recently
from
5edbaa6 to
e79e42e
Compare
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
3 times, most recently
from
c5e134d to
c9fbc91
Compare
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
3 times, most recently
from
8890b16 to
603f27a
Compare
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
2 times, most recently
from
0591d43 to
989dd3d
Compare
|
I'll follow up with changes to |
| expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) | ||
|
|
||
| push.send(:request, Net::HTTP::Post, registry) | ||
| end | ||
|
|
||
| context 'for a 3xx response' do |
There was a problem hiding this comment.
I was on the fence about making shared examples for these. On the one hand there's 3 very similar tests that can be parameterised. On the other, there are exactly 3 cases and that won't grow.
Overall I'm not a huge fan of these tests. One thought I had to reduce the level of repetition is to pull request and http into let blocks with all the appropriate method stubs set up. After that, each test could set the up the method call expectations that it specifically cares about (e.g. you'd only set up the use_ssl expectation in the test for SSL).
With that change, there would be a lot less duplication in the 3 tests, and I think they'd feel better for it.
If you think that'd be better I'm happy to follow up either with another commit on this branch, or with a separate refactor PR.
There was a problem hiding this comment.
I think this is fine?
i'm not a big fan of "shared examples", they tend to make things harder to both read and modify.
Agree you can extract those two very mocked objects to centralize them, if you feel strongly about it. Otherwise, I'd leave this as is until one of those blocks needs to be modified 3 times, and then i'd extract.
There was a problem hiding this comment.
Yeah, shared examples specifically I'm not a huge fan of either. I think I'm going to go the route of extracting as much of the duplication as possible and not setting up the same mock expectations in every test.
I'll do that in a separate PR though, so we can get this one merged.
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
3 times, most recently
from
cf75af5 to
8b9e9a8
Compare
| expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http) | ||
|
|
||
| push.send(:request, Net::HTTP::Post, registry) | ||
| end | ||
|
|
||
| context 'for a 3xx response' do |
There was a problem hiding this comment.
I think this is fine?
i'm not a big fan of "shared examples", they tend to make things harder to both read and modify.
Agree you can extract those two very mocked objects to centralize them, if you feel strongly about it. Otherwise, I'd leave this as is until one of those blocks needs to be modified 3 times, and then i'd extract.
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
6 times, most recently
from
7535413 to
120517b
Compare
This is in preparation for the introduction of arbitrary labels in the grouping key. We no longer need to support `instance` as a special case, and will instead generate a path that combines the job label with everything passed in a grouping key hash. Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
from
120517b to
a127780
Compare
This doesn't validate label keys or handle encoding special values in label values yet. That will be added in a later commit. Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
This builds on the introduction of `grouping_key` in a previous commit, ensuring that the labels passed in follow the usual naming restrictions on Prometheus labels. Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
Certain label values (empty string, and anything containing a `/`) cannot be used in the grouping key without extra work to encode them properly. Labels are represented as pairs of path segments in the URL, so an unencoded `/` would be treated as a path separator. The empty string would result in two consecturive path separators (`//`), which HTTP libraries, proxies, and web servers are liable to normalise away. This commit uses the base64 encoding method supported by the pushgateway server to encode such values. See: https://github.com/prometheus/pushgateway/blob/6393a901f56d4dda62cd0f6ab1f1f07c495b6354/README.md#url Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
The pushgateway will overwrite metric labels if the same label is used as part of the grouping key. Other Promethus clients (at least the Go one) will error if you try to do this, as it can be quite unexpected. This commit makes us follow suit. We may find that some users prefer to let those clashes through (i.e. let the labels from the grouping_key win). If that ends up being the case, we can introduce a config flag for people to opt into that behaviour. Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
Currently, we don't do anything with the response from `Net::HTTP` in the push client. This means that when we get a response that indicates our metrics didn't make it to the pushgateway, we silently carry on, and the user has no ideas that their metrics weren't recorded. If users decide they don't care about this happening, they can always rescue the base `Prometheus::Client::Push::HttpError` class (or everything that extends `StandardError` if they also want to ignore things like timeouts, connection refused, DNS resolution failure, etc). Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
from
a127780 to
ae419d2
Compare
Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
Sinjo
force-pushed
the
sinjo-push-client-improvements
branch
from
69732ba to
2105319
Compare
This PR implements the remaining improvements from #186 that weren't in #220.