Add show plan endpoint #961
Conversation
|
I already did an internal review of this function. My only outstanding comment is the duplication of code, but @saheemg decided that the simpler duplication was easier. |
api/graphite.go
Outdated
| response.Write(ctx, response.NewError(http.StatusBadRequest, err.Error())) | ||
| return | ||
| } | ||
| response.Write(ctx, response.NewJson(200, plan, "")) |
There was a problem hiding this comment.
models.GraphiteRender allows the caller to choose one out of four formats, but this will always return Json. I think it probably makes sense that this reuses the model, but for consistency there should either be some error or a correctly formatted response if another format than json was specified.
api/graphite.go
Outdated
| return | ||
| } | ||
|
|
||
| reqRenderTargetCount.Value(len(request.Targets)) |
There was a problem hiding this comment.
I think it might be confusing if this target count shows up in the same metric as the target count of actual render requests. Often when we need to analyse an MT that's in trouble due to heavy requests we rely on this metric, if showPlan() also records the evaluated number of targets in that same metric it could lead to confusion.
There was a problem hiding this comment.
I will remove this.
This is the first step in implementing #864.
It is needed now to help us at Bloomberg provide a programmatic interface to metrictank for native functions ONLY.