Different results from graphite-web

[misiek08]

Problem

Some functions in graphite-web re-fetch data while evaluating. Current architecture of carbonapi doesn't allow such operation and the results are not equal. Function like hitcount in graphite-web call evaluateTarget during evaluation. I wanted to add smartSummarize and it has same problem - the first bucket is incomplete if the start time (from) doesn't match bucket start time.

Solutions

I looked into code and I have 2 solutions in mind:

1. Change Dos signature to something like Do(ctx FunctionCallContext) ([]*types.MetricData, error) and put zipper into context, so function can fetch more data if needed
2. Add another, optional Golang function to metric function, e.g. AlignTime and it will change MetricRequests to get all data required to calculate correct values. Can be replaced by meta like AlignStartToInterval field in functions metadata.

My view at solutions

First approach would require one change affecting all functions (to add context) and then every subsequent change would benefit from this approach. This will work same as graphite-web, so there would be 2 requests for metric data. Second approach will require less changes in code (adding Align can be backward-compatible to functions not needing it), but it will add additional step to preparing metrics requests. Only one request for metrics, because time will be aligned before evaluation and first fetch.

Example code

I started implementing first approach here (with smartSummarize introduction hacking): https://github.com/misiek08/carbonapi/tree/function-context

Second approach would require looking into all functions and getting min(from) and max(until) from all functions involved in query (or smarter approach to pass those time up in the function stack and trim it).

Question

Which approach do you wonna take? I will prepare PR with smartSummarize in near future and it will be affected by same problem. Probably we can accept this first bucket inequality and that's third way to solve this problem.

[Civil]

In current master all functions accept context since 0.14.0 (that was a reason to bump version).

I don't really like idea of putting zipper into context though and I like second approach better.