Raise errors for non-2xx responses in push client

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>

lib/prometheus/client/push.rb

@@ -17,6 +17,11 @@ module Client
     # Push implements a simple way to transmit a given registry to a given
     # Pushgateway.
     class Push
+      class HttpError < StandardError; end
+      class HttpRedirectError < HttpError; end
+      class HttpClientError < HttpError; end
+      class HttpServerError < HttpError; end
+
       DEFAULT_GATEWAY = 'http://localhost:9091'.freeze
       PATH            = '/metrics/job/%s'.freeze
       SUPPORTED_SCHEMES = %w(http https).freeze
@@ -111,7 +116,10 @@ def request(req_class, registry = nil)
         req.basic_auth(@uri.user, @uri.password) if @uri.user
         req.body = Formats::Text.marshal(registry) if registry
 
-        @http.request(req)
+        response = @http.request(req)
+        validate_response!(response)
+
+        response
       end
 
       def synchronize
@@ -136,6 +144,20 @@ def validate_no_label_clashes!(registry)
           end
         end
       end
+
+      def validate_response!(response)
+        status = Integer(response.code)
+        if status >= 300
+          message = "status: #{response.code}, message: #{response.message}, body: #{response.body}"
+          if status <= 399
+            raise HttpRedirectError, message
+          elsif status <= 499
+            raise HttpClientError, message
+          else
+            raise HttpServerError, message
+          end
+        end
+      end
     end
   end
 end

spec/prometheus/client/push_spec.rb

@@ -123,6 +123,14 @@
     let(:content_type) { Prometheus::Client::Formats::Text::CONTENT_TYPE }
     let(:data) { Prometheus::Client::Formats::Text.marshal(registry) }
     let(:uri) { URI.parse("#{gateway}/metrics/job/test-job") }
+    let(:response) do
+      double(
+        :response,
+        code: '200',
+        message: 'OK',
+        body: 'Everything worked'
+      )
+    end
 
     it 'sends marshalled registry to the specified gateway' do
       request = double(:request)
@@ -134,12 +142,99 @@
       expect(http).to receive(:use_ssl=).with(false)
       expect(http).to receive(:open_timeout=).with(5)
       expect(http).to receive(:read_timeout=).with(30)
-      expect(http).to receive(:request).with(request)
+      expect(http).to receive(:request).with(request).and_return(response)
       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
+      let(:response) do
+        double(
+          :response,
+          code: '301',
+          message: 'Moved Permanently',
+          body: 'Probably no body, but technically you can return one'
+        )
+      end
+
+      it 'raises a redirect error' do
+        request = double(:request)
+        allow(request).to receive(:content_type=)
+        allow(request).to receive(:body=)
+        allow(Net::HTTP::Post).to receive(:new).with(uri).and_return(request)
+
+        http = double(:http)
+        allow(http).to receive(:use_ssl=)
+        allow(http).to receive(:open_timeout=)
+        allow(http).to receive(:read_timeout=)
+        allow(http).to receive(:request).with(request).and_return(response)
+        allow(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http)
+
+        expect { push.send(:request, Net::HTTP::Post, registry) }.to raise_error(
+          Prometheus::Client::Push::HttpRedirectError
+        )
+      end
+    end
+
+    context 'for a 4xx response' do
+      let(:response) do
+        double(
+          :response,
+          code: '400',
+          message: 'Bad Request',
+          body: 'Info on why the request was bad'
+        )
+      end
+
+      it 'raises a client error' do
+        request = double(:request)
+        allow(request).to receive(:content_type=)
+        allow(request).to receive(:body=)
+        allow(Net::HTTP::Post).to receive(:new).with(uri).and_return(request)
+
+        http = double(:http)
+        allow(http).to receive(:use_ssl=)
+        allow(http).to receive(:open_timeout=)
+        allow(http).to receive(:read_timeout=)
+        allow(http).to receive(:request).with(request).and_return(response)
+        allow(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http)
+
+        expect { push.send(:request, Net::HTTP::Post, registry) }.to raise_error(
+          Prometheus::Client::Push::HttpClientError
+        )
+      end
+    end
+
+    context 'for a 5xx response' do
+      let(:response) do
+        double(
+          :response,
+          code: '500',
+          message: 'Internal Server Error',
+          body: 'Apology for the server code being broken'
+        )
+      end
+
+      it 'raises a server error' do
+        request = double(:request)
+        allow(request).to receive(:content_type=)
+        allow(request).to receive(:body=)
+        allow(Net::HTTP::Post).to receive(:new).with(uri).and_return(request)
+
+        http = double(:http)
+        allow(http).to receive(:use_ssl=)
+        allow(http).to receive(:open_timeout=)
+        allow(http).to receive(:read_timeout=)
+        allow(http).to receive(:request).with(request).and_return(response)
+        allow(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http)
+
+        expect { push.send(:request, Net::HTTP::Post, registry) }.to raise_error(
+          Prometheus::Client::Push::HttpServerError
+        )
+      end
+    end
+
     it 'deletes data from the registry' do
       request = double(:request)
       expect(request).to receive(:content_type=).with(content_type)
@@ -149,7 +244,7 @@
       expect(http).to receive(:use_ssl=).with(false)
       expect(http).to receive(:open_timeout=).with(5)
       expect(http).to receive(:read_timeout=).with(30)
-      expect(http).to receive(:request).with(request)
+      expect(http).to receive(:request).with(request).and_return(response)
       expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http)
 
       push.send(:request, Net::HTTP::Delete)
@@ -168,7 +263,7 @@
         expect(http).to receive(:use_ssl=).with(true)
         expect(http).to receive(:open_timeout=).with(5)
         expect(http).to receive(:read_timeout=).with(30)
-        expect(http).to receive(:request).with(request)
+        expect(http).to receive(:request).with(request).and_return(response)
         expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http)
 
         push.send(:request, Net::HTTP::Post, registry)
@@ -189,7 +284,7 @@
         expect(http).to receive(:use_ssl=).with(true)
         expect(http).to receive(:open_timeout=).with(5)
         expect(http).to receive(:read_timeout=).with(30)
-        expect(http).to receive(:request).with(request)
+        expect(http).to receive(:request).with(request).and_return(response)
         expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http)
 
         push.send(:request, Net::HTTP::Put, registry)