update Go version; add websocket / correct endpoints to 'src gateway benchmark' #1125
Conversation
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
| gatewayWebsocket, sourcegraphWebsocket *websocket.Conn | ||
| err error | ||
| httpClient = &http.Client{} | ||
| endpoints = map[string]any{} |
There was a problem hiding this comment.
I think this would help clarify that any.
| endpoints = map[string]any{} | |
| endpoints = map[string]any{} // Values: URL `string`s or `*websocket.Conn`s |
| endpoints = map[string]any{} | ||
| ) | ||
| if *gatewayEndpoint != "" { | ||
| wsURL := strings.Replace(fmt.Sprint(*gatewayEndpoint, "/v2/websocket"), "http", "ws", 1) |
There was a problem hiding this comment.
Ah, the http → ws replacement is nice and elegant to keep that optional s. Self-explanatory once I saw it, but I missed it in my original version. Thanks! 👏
| endpoints["wss: gateway"] = gatewayWebsocket | ||
| endpoints["https: gateway"] = fmt.Sprint(*gatewayEndpoint, "/v2") |
There was a problem hiding this comment.
Nit, maybe this would be more precise? We output these names, so might be worth adjusting.
| endpoints["wss: gateway"] = gatewayWebsocket | |
| endpoints["https: gateway"] = fmt.Sprint(*gatewayEndpoint, "/v2") | |
| endpoints["ws(s): gateway"] = gatewayWebsocket | |
| endpoints["http(s): gateway"] = fmt.Sprint(*gatewayEndpoint, "/v2") |
(same for sourcegraph endpoints below)
There was a problem hiding this comment.
I think this update messed up something, see the check annotations below. Also, would be nice to mention in the PR description that you updated packages, and state the reason (incl. whether it was related or unrelated to the main purpose of the PR).
| defer func(Body io.ReadCloser) { | ||
| err := Body.Close() | ||
| defer func(body io.ReadCloser) { | ||
| err := body.Close() |
There was a problem hiding this comment.
Ah, thanks! This was IntelliJ's autogenerated code. Will watch out for this in the future.
| if resp.StatusCode != http.StatusOK { | ||
| fmt.Printf("non-200 response: %v\n", resp.Status) | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Maybe check the body as well? I think it's a great call that you checked for response body == "pong" for WebSockets. I think we should do the same here. I'll adjust the endpoints to respond with "pong"
|
|
||
| func benchmarkEndpointWebSocket(conn *websocket.Conn) time.Duration { |
There was a problem hiding this comment.
Naming: I'd have called this benchmarkEndpointWebsocket with a non-capital "s". I looked up the Go docs and your version seems more idiomatic with Go.
If we agree that "WebSocket" is the correct capitalization for entity names in Go then please also rename gatewayWebsocket and sourcegraphWebsocket accordingly. (I'd do it to fix my mistake but I don't want to mess or conflict with your PR.)
There was a problem hiding this comment.
I've changed it to "WebSocket" everywhere in my other PR https://github.com/sourcegraph/sourcegraph/pull/1526
There was a problem hiding this comment.
wikipedia says WebSocket so yea let's go with that capitalization
| fmt.Printf("WebSocket read error: %v\n", err) | ||
| return 0 | ||
| } | ||
| if string(message) != "pong" { |
There was a problem hiding this comment.
I've changed my other PR https://github.com/sourcegraph/sourcegraph/pull/1526 to respond with "pong" on all three endpoints.
Continues development of #1124
/.api/gatewayand/.api/gateway/websocket./v2and/v2/websocket-gatewayparameter andSRC_ENDPOINT/SRC_ACCESS_TOKENThis should be sufficient for testing against the actual production endpoints once they are added / working / live in prod.
Test plan
Not tested yet since endpoints are not live yet. Command not in use yet, so no risk.