Feature/benchmarking #56
Conversation
There was a problem hiding this comment.
I don't see anything blocking. Very clean and organized as usual. I added a few comments. A bigger question though, is that it appears that you are monitoring the queries, and that you have built everything needed to make this happen. Why not use something that already exists? We use new relic in internal-apis. It seems more straight forward than reimplementing this sort of thing. Either way it appears that its already built but maybe something to consider in the future to use other libraries.
Another note, your channels in the collector are all blocking. I just wanted to make sure that was your intention.
app/proxy/service.go
Outdated
| paramsMap[k] = v | ||
| } | ||
| return paramsMap | ||
| } else if paramsMap, ok := q.Params().(map[string]interface{}); ok { |
There was a problem hiding this comment.
won't this always be true? if you are returning a map[string]interface{} always why would we ever try to do map[string]string? Obviously, you did it for a reason. More of a curiosity for my own knowledge.
There was a problem hiding this comment.
it wouldn't be true in two cases:
- params is
nil - if
map[string]string{}{"account_id": id}was attached to params later. which I was doing. but I've changed that and now the first clause is no longer needed. thanks for prompting me to think more about this piece of code, I think it's improved now.
app/proxy/service.go
Outdated
|
|
||
| if r.Error == nil { | ||
| c.service.logger.LogSuccessfulQuery(q.Method(), execTime, q.Params()) | ||
| c.service.logger.LogSuccessfulQuery(q.Method(), execTime, q.paramsForLog()) |
There was a problem hiding this comment.
A better convention for go is to check if r.Error != nil and remove the else altogether.
There was a problem hiding this comment.
there is a non-returning pathway for when there is an error as well, so if/else is required. but yeah, replaced this with more familiar != nil
| hook := logrus_test.NewLocal(svc.logger.Logger()) | ||
| c.Call(request) | ||
|
|
||
| assert.Equal(t, map[string]interface{}{"account_id": "****"}, hook.LastEntry().Data["params"]) |
There was a problem hiding this comment.
what is the meaning of ***? Is that a default account of sorts? I would probably put this into a constant so the code reads better.
There was a problem hiding this comment.
That's just a masked value so we see the field is there but the value is not revealed. It is defined as a constant in the code, just double-checking its value here in the test.
internal/monitor/monitor.go
Outdated
| "response": errorResponse, | ||
| "params": params, |
There was a problem hiding this comment.
why the move? alphabetical order is better.
I'm actually using prometheus and its libraries. This code is to decouple main logic from those libraries so they can be replaced with something entirely else in the future, like new relic or metabase. |
Mask account_id in SDK query logs
Improve metrics collector. Documentation for metrics collector is provided in the package source files, hope it is clear enough!