Tracing support #709
Tracing support #709
Conversation
Dieterbe
commented
Aug 8, 2017
•
Dieterbe
commented
|
This is looking good @Dieterbe |
api/middleware/middleware.go
Outdated
| @@ -37,6 +37,9 @@ func getOrg(req *http.Request, multiTenant bool) (int, error) { | |||
| } | |||
| orgStr := req.Header.Get("x-org-id") | |||
| if orgStr == "" { | |||
| orgStr = req.Header.Get("x-grafana-org-id") | |||
| } | |||
| if orgStr == "" { | |||
There was a problem hiding this comment.
@woodsaj thoughts on this? I did it because grafana uses that header now, and to make the dev stack work where grafana talks to MT directly
There was a problem hiding this comment.
I am ok with this. We can also pass this through graphite-web if you like, just need to update the "REMOTE_STORE_FORWARD_HEADERS in local_settings.py
There was a problem hiding this comment.
x-grafana-org-id is just going to be the internal id of the org within the grafana instance, it won't have any correlation with the grafana.com org id
even better I think. the way this is supposed to work is that the trace context gets sent along with the getData RPC call, so the peer MT can effectively add its own logs and trace items into the "parent" context from the caller, and it'll show up as 1 tree in the jaeger UI. i still have to get this to work, but that's how it should be. |
Dieterbe
force-pushed
the
tracing
branch
2 times, most recently
from
7bc2133
to
e63cc7c
Compare
|
here's a screenie of the current implementation, demonstrating the above. |
| r.TraceLog(span) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
i suspect this may be overdoing it. for every request in a GetData RPC call, log all the request properties. this could be too much overhead. but we can leave it until we see perf trouble
| @@ -27,6 +31,14 @@ func Write(w http.ResponseWriter, resp Response) { | |||
| return | |||
| } | |||
|
|
|||
| func WriteErr(ctx context.Context, w http.ResponseWriter, resp Response) { | |||
There was a problem hiding this comment.
every call to WriteErr is passing writeErr(ctx.Req.Context(), ctx, error)
so why not just use
func WriteErr(c *macaron.Context, resp Response) {
ctx := c.Req.Context()
.....
}
It may even be a better idea to just include the error logger in the Write() method, eg.
func Write(ctx *macaron.Context, resp Response) {
defer resp.Close()
body, err := resp.Body()
if err != nil {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(err.Error()))
return
}
for k, v := range resp.Headers() {
w.Header().Set(k, v)
}
w.WriteHeader(resp.Code())
w.Write(body)
// log errors
if errResp, ok := resp.(Error); ok { // or maybe "if resp.Code() >= 400"
span := opentracing.SpanFromContext(ctx)
body, _ := resp.Body()
span.LogFields(log.String("error.kind", string(body)))
tags.Error.Set(span, true)
}
}```
There was a problem hiding this comment.
I liked that the response package was pretty standalone and not tied into macaron, and wanted to keep it that way.
| @@ -0,0 +1,38 @@ | |||
| package conf | |||
There was a problem hiding this comment.
why is this package called "conf" when all handles is tracing?
There was a problem hiding this comment.
conf is not new. https://github.com/raintank/metrictank/tree/master/conf
before it was just used for extra config related things like storage-schemas and aggregation rules config, and now also for tracing config/setup. I thought about putting it in a tracing package but it would be a tiny package. though perhaps that's the better way.
