Opentracing #154
Opentracing #154
Conversation
Signed-off-by: kuba-- <kuba@sourced.tech>
Signed-off-by: kuba-- <kuba@sourced.tech>
Signed-off-by: kuba-- <kuba@sourced.tech>
Signed-off-by: kuba-- <kuba@sourced.tech>
server/config.go
Outdated
| "gopkg.in/src-d/go-vitess.v0/mysql" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
You don't need this, you can use https://github.com/jaegertracing/jaeger-client-go/blob/6cc524e2736308fb6da8f054483d00baee39b0a1/config/config_env.go#L49 to get configuration from environment variables.
There was a problem hiding this comment.
And then, with this configuration, you can create a new tracer: https://sourcegraph.com/github.com/jaegertracing/jaeger-client-go@6cc524e2736308fb6da8f054483d00baee39b0a1/-/blob/config/config.go#L148
There was a problem hiding this comment.
Yeah, I know, but it was added recently and it's still just on master. I hope after they release a new version we'll be able to get rid of some stuff and directly use heir FromEnv functions.
There was a problem hiding this comment.
Oh, ok, sorry for the noise. Can you open an issue to remind change that in the future?
There was a problem hiding this comment.
Wait, I've just checked that they released a new version 2.13 two days ago (with env config), so on Friday the latest version was 2.12 (the version which we are using right now).
So, shall we integrate a new version?
_example/main.go
Outdated
| panic(err) | ||
| } else { |
There was a problem hiding this comment.
no need for an else, panic stops execution
| } | ||
|
|
||
| func createTestDatabase() *mem.Database { | ||
| db := mem.NewDatabase("test") | ||
| db := mem.NewDatabase("test").(*mem.Database) |
There was a problem hiding this comment.
why is the casting needed?
There was a problem hiding this comment.
because mem.NewDatabase returns sql.Database interface which does not have AddTable function.
| return buffer[0], nil | ||
| span, ctx := ctx.Span("aggregation.Count_Eval") | ||
| count := buffer[0] | ||
| span.LogKV("count", count) |
There was a problem hiding this comment.
why do we need the result of the aggregation in the log?
There was a problem hiding this comment.
...just to be consistent with other operations (max, min, avg, ...) if it's too noisy we can remove it.
There was a problem hiding this comment.
Yeah, perhaps having in the logs the results of these operations is too noisy
Signed-off-by: kuba-- <kuba@sourced.tech>
|
PTAL: jagger-client-go was upgraded to 2.13.0 |
Description: #151