-
Notifications
You must be signed in to change notification settings - Fork 351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/ping response code #45
base: master
Are you sure you want to change the base?
Conversation
Now you can decide what type of http status code the ping will return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple smaller changes I'd recommend
addr string | ||
name string | ||
schema string | ||
response int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather see a name like pingResponseCode
and have it separated by a newline so that it's visually separate from the above group of related fields.
@@ -113,10 +116,15 @@ func (h *HTTP) Stop() error { | |||
func (h *HTTP) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
start := time.Now() | |||
|
|||
pingStatusResponse := DefaultHttpPingResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole block should only happen once, and not on every request.
w.Header().Add("X-InfluxDB-Version", "relay") | ||
w.WriteHeader(http.StatusNoContent) | ||
return | ||
w.Header().Add("X-InfluxDB-Version", "relay") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is going to return codes other than 204
, Content-Length
should also be set to 0
.
Hey @joelegasse, thanks for your relevant review. I agree with all your remarks, just made the changes. |
Hey there, |
Also Amazon ELB requires 200 and does not work with 204. |
Getting an error with this PR:
Fix:
|
Soooo...this is still an issue. Google gcp still only accepts 200 response codes. This has been open since 2016 and never fixed? It has been adjusted in both influxdb and telegraf. See this issue: https://github.com/influxdata/telegraf/pull/5704/files |
Hey guys,
We had a problem regarding the hardcoded
204 No Content
of/ping
response code.Indeed, Google Cloud Plateform
http health-checks
only accept200 OK
.With this PR, you'll be able to control what response code of
/ping
you want/need to have, from the config file, usingdefault-ping-response
.Thank you guys for this great tool!