make analytics.Client more thread-safe #283
Conversation
It's still not fully thread-safe, since we depend on being able to append properties to it in klothomain; but for individual sends, it is. We do this by separating out the client from the payload. Each time we send a new message, we create a new payload, populate it with default stuff from the client, modify that paylod (which is thread-local) and send it.
ghost
requested a review
from 
|
I found myself getting a bit lost as I was writing this (much of it yesterday, when I was a bit foggy), so please do take a look to see if I did something wonky in terms of an awkward internal API or struct usage. It's probably due to piecemeal evolving code, rather than a principled stance on code structure. |
| logLevel = Panic | ||
| } | ||
|
|
||
| p := fl.client.createPayload(logLevel, entry.Level.CapitalString()) |
There was a problem hiding this comment.
Shouldn't the event be the same (like "log") since the level is already in the properties? Seems odd to have this duplicated is all
There was a problem hiding this comment.
I think I prefer this a bit, just because it makes it a bit easier as you're scanning the all-events dashboard to visually pick out warnings vs logs.
|
|
||
| return nil | ||
| if err != nil { | ||
| zap.L().Debug(fmt.Sprintf("Failed to send metrics info. %v", err)) |
There was a problem hiding this comment.
This could cause infinite loop if the server is down. Also, the previous version returning the error to let the caller decide what to do makes the most sense IMO.
There was a problem hiding this comment.
The infinite loop was there before, too :-\ I noticed it but didn't feel like thinking about how to clean it up, heh.
Regarding returning the error: in the previous version, there was only one caller,(*Client).track:
func (t *Client) track(event string) {
t.Event = event
err := SendTrackingToServer(t)
if err != nil {
zap.L().Debug(fmt.Sprintf("Failed to send metrics info. %v", err))
}
}
So:
SendTrackingToServerreturns a possible errorTrackis the only invoker ofSendTrackingToServer, and the only real value it provides is to turn that error into a log line
It felt like inlining those made sense. That said, maybe it would be clearer if I (a) un-exported SendTrackingToServer and (b) moved it into client.go?
- un-export `send` - have `createPayload` and `send` use the Payload directly (not pointer) - create `AppendProperty` - tweak Sprintf call
|
It's still not fully thread-safe, since we depend on being able to
append properties to it in klothomain; but for individual sends, it is.
We do this by separating out the client from the payload. Each time we
send a new message, we create a new payload, populate it with default
stuff from the client, modify that paylod (which is thread-local) and
send it.
This PR has two commits (at least for rev1):
client_test.goline 91: when we send analytics directly (not via the logger hook), we now send"status": "info"where we didn't beforeFollowup to #281.
Standard checks