-
Notifications
You must be signed in to change notification settings - Fork 47
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
Better tracking ConnectedPlayers #48
Conversation
14faeb0
to
f96aeea
Compare
Some minor comments and nits, but otherwise LGTM! |
metadata: | ||
annotations: | ||
controller-gen.kubebuilder.io/version: v0.4.1 | ||
creationTimestamp: null |
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.
Nit: this field can be removed, since it is null.
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 is auto-generated when running make
. Probably the controller-gen utility does it :(
sidecar-go/httphandler.go
Outdated
|
||
fmt.Printf("Got values from allocation, sessionID:%s, sessionCookie:%s, initialPlayers:%#v\n", sessionID, sessionCookie, initialPlayers) | ||
initialPlayers := h.getInitialPlayersDetails() | ||
log.Infof("Got values from allocation, initialPlayers:%#v", initialPlayers) |
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.
You could be referencing a nil slice here, if there was an error in getInitialPlayerDetails
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.
ah gotcha - will this panic though?
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.
shouldn't, because you are looking at the slice itself, vs a property of it.
sidecar-go/httphandler.go
Outdated
@@ -157,34 +183,41 @@ func (h *httpHandler) heartbeatHandler(w http.ResponseWriter, req *http.Request) | |||
} | |||
|
|||
if logEveryHeartbeat { | |||
fmt.Printf("heartbeat received from sessionHostId %s, data %#v\n", sessionHostId, hb) | |||
log.Infof("heartbeat received from sessionHostId %s, data %#v", sessionHostId, hb) |
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.
Suggestion: For outputing regular "succcess" or "it works" text, consider the debug severity. Info has always felt to me to be used for significant events in a system, which don't happen frequently.
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.
done. Should we have an env variable specifying the log level?
|
||
func (h *httpHandler) updateConnectedPlayersCountIfNeeded(ctx context.Context, hb *HeartbeatRequest) error { |
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.
Having a player count metric exposed may be useful as well, so that prometheus can track and graph this in grafana. totally something that can be done in a follow on PR.
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.
awesome idea, created #49 to track
This PR does a bunch of things.
I also wanted to rename the httphandler struct and file as part of #37, but I'll do that on a separate PR, since this one is already big.