go-graphite / carbonapi

Implementation of graphite API (graphite-web) in golang
Other
309 stars 140 forks source link

windowSize was 0 from movingMax #371

Closed msaf1980 closed 6 years ago

msaf1980 commented 6 years ago

On query with long data range (over 30 days)

render/?format=json&from=-6mon&target=movingMax(minSeries(elasticsearch.{vm-elk6-s1,vm-elk6-s3,vm-elk6-s2,vm-elk6-s4}.*.cluster_health.status),'3min')&until=now

rasied runtime error: index out of range

github.com/go-graphite/carbonapi/expr/types.(Windowed).Push ~/go/src/github.com/go-graphite/carbonapi/expr/types/windowed.go:26 github.com/go-graphite/carbonapi/expr/functions/moving.(moving).Do ~/go/src/github.com/go-graphite/carbonapi/expr/functions/moving/function.go:109 github.com/go-graphite/carbonapi/expr.EvalExpr ~/go/src/github.com/go-graphite/carbonapi/expr/expr.go:46 main.renderHandler.func2 ~/go/src/github.com/go-graphite/carbonapi/cmd/carbonapi/http_handlers.go:364

Some variables: n = 180 int(arg[0].StepTime) = 600

windowSize = 180 / 600 = 0

So, empty slice was allocated at

w := &types.Windowed{Data: make([]float64, windowSize)}

How handle this in correct way ?

msaf1980 commented 6 years ago

Simple workaround for this - set windowSize = 1

if scaleByStep {
    windowSize /= int(arg[0].StepTime)
    // Fix downsampling error (empty w) on long time ranges
    // https://github.com/go-graphite/carbonapi/issues/371
    if windowSize == 0 {
        windowSize = 1
    }
    offset = windowSize
}
Civil commented 6 years ago

The correct way would be to handle this like it's done in Graphite-web: https://github.com/graphite-project/graphite-web/blob/master/webapp/graphite/render/functions.py#L1196-L1266

The bug you've found happens when the window is a string (e.x. "10m").

As far as I can see we are doing in that case something completely not correct, so it's better to understand what graphite-web is doing and rewrite it the same way.

Or another way to fix - setup graphite-web and see how it's behaves in the same situation.

msaf1980 commented 6 years ago

graphite-web return array of NaN values in this case