Avoid final garbage value from Fetch()

[thepaul]

I'm not 100% certain this is the right fix for everyone- I haven't checked the librrd api or anything, but at least for my use case I always get one garbage value at the end of my Fetch() results. Taking out this "+ 1" makes the data match expectations.

[hnakamur]

Fetch() calls the C function rrd_fetch_r() in rrdtool https://github.com/oetiker/rrdtool-1.x/blob/1.4.8/src/rrd_fetch.c#L183-L203 and rrd_fetch_r() calls rrd_fetch_fn() https://github.com/oetiker/rrdtool-1.x/blob/1.4.8/src/rrd_fetch.c#L205-L483

In rrd_fetch_fn(), one malloc() is called for allocating a memory for two dimensional data (dsCnt * rows * sizeof(rrd_value_t)) https://github.com/oetiker/rrdtool-1.x/blob/1.4.8/src/rrd_fetch.c#L340-L362

Since we are mapping Values []float64 in FetchResult to the malloc'ed memory, we must use the same dsCnt and rows values. https://github.com/ziutek/rrd/blob/f3b7823b2eda557b02d5b92028cc7ab0f7452405/rrd.go#L396-L405 https://github.com/ziutek/rrd/blob/f3b7823b2eda557b02d5b92028cc7ab0f7452405/rrd_c.go#L444-L451

I skimmed the rrd_fetch_fn() source code, https://github.com/oetiker/rrdtool-1.x/blob/1.4.8/src/rrd_fetch.c#L205-L483 but I could not figure out the last data at rows is garbage or not.

I think you can skip reading the last data without this pull request.

[thepaul]

Sure, you can skip reading the last data if you know about the problem. There is a workaround.

But handing back uninitialized values is usually not considered great library behavior.

[hnakamur]

I checked the rrdtutorial example and noticed rrdtool fetch test.rrd AVERAGE --start 920804400 --end 920809200 prints nan at 920809200. I also tested it myself with rrdtool 1.3.8 and confirmed this behavior.

The tutorial saids

The number that is written behind 920809200: in the list above covers the time range from 920808900 to 920809200, EXCLUDING 920809200.

Since Fetch() is meant to get the same result as rrdfetch, I think it's OK as it is. But I leave the final decision to @ziutek

[thepaul]

I wasn't getting NaN, though. I was getting random garbage values.

[thepaul]

this may be convincing:

t.go

package main

import (
    "fmt"
    "os"
    "strconv"
    "time"

    "github.com/ziutek/rrd"
)

func main() {
    start, _ := strconv.Atoi(os.Args[2])
    end, _ := strconv.Atoi(os.Args[3])
    result, err := rrd.Fetch(os.Args[1], "AVERAGE", time.Unix(int64(start), 0),
        time.Unix(int64(end), 0), time.Second)
    if err != nil {
        panic(err)
    }
    for i := 0; i < result.RowCnt; i++ {
        fmt.Printf(" %d: %.10e\n",
            result.Start.Add(time.Duration(i+1)*result.Step).Unix(),
            result.ValueAt(0, i))
    }
}

output of rrdtool fetch on the test.rrd from the rrdtutorial example:

~% rrdtool fetch test.rrd AVERAGE --start 920804400 --end 920809200
                          speed

 920804700: -nan
 920805000: 4.0000000000e-02
 920805300: 2.0000000000e-02
 920805600: 0.0000000000e+00
 920805900: 0.0000000000e+00
 920806200: 3.3333333333e-02
 920806500: 3.3333333333e-02
 920806800: 3.3333333333e-02
 920807100: 2.0000000000e-02
 920807400: 2.0000000000e-02
 920807700: 2.0000000000e-02
 920808000: 1.3333333333e-02
 920808300: 1.6666666667e-02
 920808600: 6.6666666667e-03
 920808900: 3.3333333333e-03
 920809200: -nan
 920809500: -nan

output of test program 't':

~% ./t test.rrd 920804400 920809200
 920804700: NaN
 920805000: 4.0000000000e-02
 920805300: 2.0000000000e-02
 920805600: 0.0000000000e+00
 920805900: 0.0000000000e+00
 920806200: 3.3333333333e-02
 920806500: 3.3333333333e-02
 920806800: 3.3333333333e-02
 920807100: 2.0000000000e-02
 920807400: 2.0000000000e-02
 920807700: 2.0000000000e-02
 920808000: 1.3333333333e-02
 920808300: 1.6666666667e-02
 920808600: 6.6666666667e-03
 920808900: 3.3333333333e-03
 920809200: NaN
 920809500: NaN
 920809800: 0.0000000000e+00

..and in some cases (not yet sure which) the last value is something besides 0.0.

[hnakamur]

@thepaul Yes, I'm convinced now. Thanks for your test :-)

Could you test your pull request for a case with dsCnt > 1?