support consolidateBy #641
Conversation
Dieterbe
force-pushed
the
consolidateBy
branch
3 times, most recently
from
efc17c0
to
4d674a4
Compare
|
I like this a lot, the only issue I see right now is that queries will operate differently if they can be fully answered by MT vs being passed off to graphite. It could be interesting to look at extending graphite to have the same concept of an aggregation method for each TimeSeries, though I'm not sure how much work that would entail. |
so that we can fetch the right archives consolidateBy -> context -> expr.Req -> models.Req -> models.Series
Dieterbe
force-pushed
the
consolidateBy
branch
3 times, most recently
from
263f619
to
c26c03c
Compare
useful for functions that need normalizing (currently none, see NOTES) and consolidation when generating the final response
and some more comments to make things clearer
this way you can issue requests like target=foo&target=consolidateBy(foo,"sum") which would previously get merged.
consolidation/consolidate.go
Outdated
| if len(in) == cleanLen { | ||
| out = in[0:outLen] | ||
| out_i := 0 | ||
| var next_i int |
There was a problem hiding this comment.
both out_i and next_i are initialized to 0. let's keep declaration consistent.
consolidation/consolidate.go
Outdated
| outLen += 1 | ||
| out = in[0:outLen] | ||
| out_i := 0 | ||
| var next_i int |
There was a problem hiding this comment.
both out_i and next_i are initialized to 0. let's keep declaration consistent.
consolidation/consolidate.go
Outdated
| outLen := len(in) / num | ||
| var out []schema.Point | ||
| cleanLen := num * outLen // what the len of input slice would be if it was a perfect fit | ||
| if len(in) == cleanLen { |
There was a problem hiding this comment.
It is not obvious that the purpose of the the 2 code paths is simply to handle aggNum dividing into len(in) evenly or not.
Can you add some more comments.
consolidation/consolidate.go
Outdated
| var next_i int | ||
| for in_i := 0; in_i < cleanLen; in_i = next_i { | ||
| next_i = in_i + num | ||
| out[out_i] = schema.Point{Val: aggFunc(in[in_i:next_i]), Ts: in[next_i-1].Ts} |
There was a problem hiding this comment.
why is the output using the timestamp of the last point, and not the first? When graphite normalizes data, it use the TS of the first point.
https://github.com/graphite-project/graphite-web/blob/master/webapp/graphite/render/functions.py#L145-L147
There was a problem hiding this comment.
when measurements need to be consolidated, and those measurements contain a spike in the signal for example, than it would be incorrect to move that spike to an earlier timestamp than when it actually occured. when troubleshooting and correlating, people expect that the signal may be delayed, but if it can also move back into time things get confusing and unreliable.
after some thinking and experimentation, I think I came up with a pretty elegant approach.
@woodsaj @DanCech please have a look at expr/NOTES, read it carefully and let me know whether you agree with every sentence. I think it describes a pretty elegant approach that will address a lot of our shortcomings.