issues wrt reusing expr library

[Dieterbe]

Hello, for our metrictank graphite backend, I'm interested in adding graphite target expression parsing and evaluation, so I tried leveraging the recently standalone-ized carbonapi expr package (#145), but I ran into some snags:

1) isNameChar(r byte) bool seems too restrictive. we have metricnames that contain other chars like \ and would also like to use = (so we can use metrics2.0). #29 seems to be about the same problem.

2) some functions can use some documentation/explaining, especially the commonly used ones. like what is SortMetrics for? why do we need this?

3) our system fetches data from different archives based on consolidateBy, so that data is always accurate when combining runtime consolidation and using rolled up archives (see graphite gotcha 6). but the expr package only sets it during EvalExpr, which requires the fetched data.

4) the expr library is currently uses its MetricData type throughout, which:

• has various options that deal with rendering options for cairo, which we don't have, and arguably don't belong in metrics data structures.
• embeds the FetchResponse type which looks a bit un-generic and tied to serialisation implementation details specific to the booking stack (e.g. pointer types for timestamps, the XXX_unrecognized seems out of place too)

having to use the MetricData type for all data, to use the expr evalutation is a bit clunky at best, hard/suboptimal at worst. Not sure what is the right solution. maybe using an interface ? perhaps like:

foo type interface {
      Next() (val float64, ts uint32)
      Len() int
}

or should we avoid the interface overhead? Our data format is currently:

package schema

type Point struct {
    Val float64
    Ts  uint32
}

package models

type Series struct {
    Target     string // will be set to the target attribute of the given request
    Datapoints []schema.Point
    Interval   uint32
}

We would of course like to be able to use our format. One change that may make sense to make on our end is instead of storing the unix timestamp for each point, just store the begin and step and the list of values. and maybe the end timestamp ? this would make it a bit more compatible with how this library works. we may also have to implement the isAbsent array concept because currently we use math.NaN() which conflates the concept of NaN and null. not perfect but works well enough, but this also looks like a current incompatibility. (note: funnily, i see that carbonapi also treats NaN and null (absent) the same (renders null) in the json output, but that's presumably just because of what graphite clients expect. i envision in graphite-ng we can have null as well as nan in the output)

5) getSeriesArg is called when evaluating (i.e. after fetching the data). we should do this while parsing the expression, so we can return errors early to user, instead of doing all the heavy work first

anyway, just a heads up at this point. I'm not asking you to make these changes at this point, we're figuring out our options, but thought you should know at least what stumbling blocks are for people trying to reuse the expr library

[dgryski]

Just pushed the tiniest cleanup which moves all the graph rendering options and functions into cairo.go, which is a bit cleaner.

I'll think about different formats for the data. the MetricData type is wrapping the protobuf messages (which explains the pointer oddities). Some of that would go away if/when we switch to protobuf v3.

The NaN/null stuff is annoying, but (to my mind) currently unavoidable. Note that there is no NaN value in JSON.

Part of the issue is that the we need to be able to create entirely new series, and passing in an interface means we don't know what concrete type to return so that the caller is happy. That will take some figuring out too.

I will continue to work on this.

[Civil]

Btw, as we've migrated to protobuf3, some of the annoyance (pointers) should go away.

[Civil]

Should be much better after https://github.com/go-graphite/carbonapi/commit/9564acbd8c191d1f2d3d1526b1c99f92bc336d3a change.

This change splits expression library into multiple functions, decouples expression parser from evaluator, etc.