Fix response code check since pushgateway success response code changed from 202 to 200

adriananeci commented 5 years ago

Q	A
Bug fix?	no	yes
New feature?	no	yes
API breaks?	no	yes
Deprecations?	no	yes
Related tickets	https://github.com/prometheus/pushgateway/pull/290
License	Apache 2.0

What's in this PR?

Change response code check from HttpURLConnection. HTTP_ACCEPTED to HttpURLConnection.HTTP_OK since pushgateway success response code changed starting from version 0.10.0

Why?

Avoid throwing IOException when metrics are successfully sent to pushgateway

Additional context

Checklist

[ x] Error handling code meets the guideline
[ ] Logging code meets the guideline (TODO)
[ ] User guide and development docs updated (if needed)
[ ] Related Helm chart(s) updated (if needed)

To Do

[ ] If the PR is not complete but you want to discuss the approach, list what remains to be done here

sancyx commented 5 years ago

Hi @adriananeci. As I see this change is release in version v0.10.0 (https://github.com/prometheus/pushgateway/pull/290). I'm wondering if we should check for 200 and 202 as well, to support older versions, or may be better to check if it's equal to 400, that would be en error for sure?

adriananeci commented 5 years ago

Hi @sancyx. Indeed, in order to maintain backward compatibility with older versions of pushgateway we should keep both response codes (202 and 200) as valid. I've adjusted the code accordingly. Not sure if checking if is equal with 400 is a good idea since you can get many other error response codes( e.g if pushgateway is behind a proxy, etc).

sancyx commented 5 years ago

@adriananeci, thanks for PR

Drewster727 commented 4 years ago

Did this commit/build make it's way into the maven repo release? https://raw.github.com/banzaicloud/spark-metrics/master/maven-repo/releases I'm still seeing an error in the logs. I'm using pushgateway 1.0.0, I see this exception consistently (but still get metrics):

java.io.IOException: Response code from http://<redacted>:9091//metrics/job/application_1576627564424_0147/role/driver/instance/<redacted>/cluster/360/environment/test/app_name/SparkHiveSessionBuilder%24/type/hadoop/worker/worker1 was 200
    at com.banzaicloud.metrics.prometheus.client.exporter.PushGatewayWithTimestamp.doRequest(PushGatewayWithTimestamp.java:252)
    at com.banzaicloud.metrics.prometheus.client.exporter.PushGatewayWithTimestamp.pushAdd(PushGatewayWithTimestamp.java:168)
    at com.banzaicloud.spark.metrics.sink.PrometheusSink$Reporter.report(PrometheusSink.scala:126)
    at com.codahale.metrics.ScheduledReporter.report(ScheduledReporter.java:162)
    at com.codahale.metrics.ScheduledReporter$1.run(ScheduledReporter.java:117)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)

stoader commented 4 years ago

This was released with version 2.3-2.1.2

/cc @sancyx

Drewster727 commented 4 years ago

thanks @stoader -- I'll check the maven releases from now on, as I did not see that version under the github repo releases: https://github.com/banzaicloud/spark-metrics/releases

banzaicloud / spark-metrics