Skip to content
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

api/etcdhttp: serve error information in '/health', marshal health in JSON #8312

Merged
merged 2 commits into from
Jul 27, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jul 26, 2017

No description provided.

}

func (h health) marshal() string {
s := fmt.Sprintf(`{"health": "%v"`, h.Health)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably just use json encoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then {"health": "true"} becomes {"health":true}. Possibly break backward compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not sure if this is that important... the previous "true" is a mistake. we should change it to either "health": "ok" or "health": true. @heyitsanthony opinion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt it can be safely changed.

@gyuho gyuho changed the title api/etcdhttp: serve error information in '/health' api/etcdhttp: serve error information in '/health', marshal health in JSON Jul 26, 2017
@gyuho
Copy link
Contributor Author

gyuho commented Jul 26, 2017

@xiang90 @heyitsanthony PTAL. Thanks.

@@ -57,7 +58,7 @@ func newHealthHandler(srv *etcdserver.EtcdServer) http.HandlerFunc {
return
}
h := checkHealth(srv)
d := []byte(fmt.Sprintf(`{"health": "%v"}`, h.Health))
d, _ := json.Marshal(h)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a TODO about removing {"health" : "true"} from etcdctl cluster-health in the future; I don't think it can be done in the same minor rev without breaking compat

@@ -45,12 +45,12 @@ It is important to monitor your production etcd cluster for healthy information

#### Health Monitoring

At lowest level, etcd exposes health information via HTTP at `/health` in JSON format. If it returns `{"health": "true"}`, then the cluster is healthy. Please note the `/health` endpoint is still an experimental one as in etcd 2.2.
At lowest level, etcd exposes health information via HTTP at `/health` in JSON format. If it returns `{"health":true}`, then the cluster is healthy. Please note the `/health` endpoint is still an experimental one as in etcd 2.2.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really still experimental? It seems people depend on cluster-health which depends on /health

@gyuho
Copy link
Contributor Author

gyuho commented Jul 27, 2017

@heyitsanthony Added TODO on health and removed experimental.... PTAL.

Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after rpctypes fixup; thanks

return h
}

if uint64(srv.Leader()) == raft.None {
h.Errors = append(h.Errors, rpctypes.ErrNoLeader.Error())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should be etcdserver.ErrNoLeader to avoid the rpctypes dep

… JSON

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants