{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6702","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6702/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6702/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6702/events","html_url":"https://github.com/cockroachdb/cockroach/pull/6702","id":155071735,"number":6702,"title":"server: refactor most of status endpoints to be provided over grpc with grpc-gateway for http","user":{"login":"d4l3k","id":909104,"avatar_url":"https://avatars.githubusercontent.com/u/909104?v=3","gravatar_id":"","url":"https://api.github.com/users/d4l3k","html_url":"https://github.com/d4l3k","followers_url":"https://api.github.com/users/d4l3k/followers","following_url":"https://api.github.com/users/d4l3k/following{/other_user}","gists_url":"https://api.github.com/users/d4l3k/gists{/gist_id}","starred_url":"https://api.github.com/users/d4l3k/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/d4l3k/subscriptions","organizations_url":"https://api.github.com/users/d4l3k/orgs","repos_url":"https://api.github.com/users/d4l3k/repos","events_url":"https://api.github.com/users/d4l3k/events{/privacy}","received_events_url":"https://api.github.com/users/d4l3k/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":7,"created_at":"2016-05-16T16:53:57Z","updated_at":"2016-05-16T21:37:05Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/6702","html_url":"https://github.com/cockroachdb/cockroach/pull/6702","diff_url":"https://github.com/cockroachdb/cockroach/pull/6702.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/6702.patch"},"body":"This fixes most of #5530 by proxying requests over the standard grpc connection instead of the http admin interface on a different port.\r\n\r\nMost of these changes are automatically generated protobuf output.\r\n\r\nConverted endpoints:\r\n* `/_status/details/{node_id}`\r\n* `/health`\r\n* `/_status/nodes`\r\n* `/_status/nodes/{node_id}`\r\n* `/_status/ranges/{node_id}`\r\n* `/_status/gossip/{node_id}`\r\n\r\nThings that remain as http and will be converted in a later PR:\r\n* `/_status/logs/{node_id}`\r\n* `/_status/logfiles/{node_id}`\r\n* `/_status/logfiles/{node_id}/{file}`\r\n* `/_status/metrics/{node_id}`\r\n* `/_status/stacks/{node_id}` - Will probably remain as just HTTP.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/6702)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/219555964","html_url":"https://github.com/cockroachdb/cockroach/pull/6702#issuecomment-219555964","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6702","id":219555964,"user":{"login":"tamird","id":1535036,"avatar_url":"https://avatars.githubusercontent.com/u/1535036?v=3","gravatar_id":"","url":"https://api.github.com/users/tamird","html_url":"https://github.com/tamird","followers_url":"https://api.github.com/users/tamird/followers","following_url":"https://api.github.com/users/tamird/following{/other_user}","gists_url":"https://api.github.com/users/tamird/gists{/gist_id}","starred_url":"https://api.github.com/users/tamird/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tamird/subscriptions","organizations_url":"https://api.github.com/users/tamird/orgs","repos_url":"https://api.github.com/users/tamird/repos","events_url":"https://api.github.com/users/tamird/events{/privacy}","received_events_url":"https://api.github.com/users/tamird/received_events","type":"User","site_admin":false},"created_at":"2016-05-16T21:37:05Z","updated_at":"2016-05-16T21:37:05Z","body":"\n\n\n\n\n\nReviewed 13 of 13 files at r5, 8 of 8 files at r6.\nReview status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.\n\n---\n\n*[gossip/gossip.go, line 551 \\[r2\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6702#-KHuZCqRnR6QFzTfX47S:-KHvUuW-Myi6h1WeegyA:1170911349) ([raw file](https://github.com/cockroachdb/cockroach/blob/929a323e7a4348a798e0722f6857c12fa0478fcd/gossip/gossip.go#L551)):*\n<details><summary><i>Previously, d4l3k (Tristan Rice) wrote…</i></summary>\n> You mean why *InfoStatus instead of InfoStatus?\n> \n> GRPC expects a pointer to be returned and not by value. Seems simplest to have it be a pointer all the way through.</details>\nI'd prefer a value - no need to put it on the heap until it's necessary - this method is quite far from the grpc use point.\n\n---\n\n*[server/server.go, line 551 \\[r5\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6702#-KHvR70_FIIH03ULLDGw:-KHvR70a76FCY8w1pWom:506948623) ([raw file](https://github.com/cockroachdb/cockroach/blob/2c2abc1400e234206a116a192e0ff050f879231a/server/server.go#L551)):*\n> ```Go\n> \n> // Close closes all grpc-gateways.\n> func (s *Server) Close() {\n> ```\n\nThis doesn't appear to be used. I guess you moved it here because the admin server had a similar method, but you never hooked this one up to the stopper above.\n\nInstead of this method, I suggest you just `stopper.AddCloser(stop.CloserFn(gwCancel))` above. I think with that change you'll be able to remove the gwCancel member from the server struct.\n\n---\n\n*[server/status.proto, line 37 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6702#-KHuYexCmlV_zYxzNznc:-KHvUYm6l6cy_GxrbBAl:453675813) ([raw file](https://github.com/cockroachdb/cockroach/blob/92113eca3ee340742a9abcb3edffafc043582cd1/server/status.proto#L37)):*\n<details><summary><i>Previously, d4l3k (Tristan Rice) wrote…</i></summary>\n> Turns out it's not used in the UI at all. \n> \n> I'm not entirely sure why it was named buildInfo in the first place. I can't find any other references to that name.</details>\nIs it really not used at all? Let's remove it then.\n\n---\n\n*[server/status.proto, line 44 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6702#-KHuYp2OtKd-LDvzhKkg:-KHvUhDToT0Y0qFxRBni:631357423) ([raw file](https://github.com/cockroachdb/cockroach/blob/92113eca3ee340742a9abcb3edffafc043582cd1/server/status.proto#L44)):*\n<details><summary><i>Previously, d4l3k (Tristan Rice) wrote…</i></summary>\n> That's what the existing code does.\n> \n> https://github.com/cockroachdb/cockroach/blob/master/util/http.go#L150\n> \n> I can change it to be \n> ```\n> repeated status.NodeData data = 1 [(gogoproto.jsontag) = \"d\"];\n> ```\n> But that seems bad.</details>\nI'd suggest changing the UI to match a more reasonable name. The existing code does that because it's a \"generic\" wrapper.\n\n---\n\n*[server/status.proto, line 36 \\[r4\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6702#-KHvUeLW4IQ1MnQbxtwk:-KHvUeLW4IQ1MnQbxtwl:1656608970) ([raw file](https://github.com/cockroachdb/cockroach/blob/7be3e1c9ec9b2ae0482a2c776a13d428337e8495/server/status.proto#L36)):*\n> ```protobuf\n> \n> message DetailsResponse {\n>   int32 node_id = 1 [(gogoproto.customname) = \"NodeID\", (gogoproto.jsontag) = \"nodeID\",\n> ```\n\nhow about this json tag, can we remove it?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/6702)*\n<!-- Sent from Reviewable.io -->\n"}}