segmentio / stats

Go package for abstracting stats collection
https://godoc.org/github.com/segmentio/stats
MIT License
208 stars 32 forks source link

fix typo in splitMeasureField() #90

Closed yerden closed 5 years ago

yerden commented 6 years ago

Current splitMeasureField() implementation leads to non-equivalent behaviour of old-fashioned stats.Incr() and new stats.Report() APIs.

yerden commented 6 years ago

This may be proven by this piece of code:

package main

import (
    "flag"
    "github.com/segmentio/stats"
    "github.com/segmentio/stats/prometheus"
    "net/http"
)

type myMetrics struct {
    dice struct {
        vertex struct {
            rolls  int `metric:"rolls" type:"counter"`
            throws int `metric:"throws" type:"counter"`
        } `metric:"vertex"`
    } `metric:"dice"`
}

var useReport = flag.Bool("use-report", false, "Use report instead of Add()")

func main() {
    flag.Parse()

    handler := prometheus.DefaultHandler
    stats.Register(handler)
    http.Handle("/metrics", handler)

    go func() {
        if *useReport {
            m := &myMetrics{}
            m.dice.vertex.rolls = 1
            m.dice.vertex.throws = 1
            stats.Report(m)
        } else {
            stats.Incr("dice.vertex.rolls")
            stats.Incr("dice.vertex.throws")
        }
    }()

    http.ListenAndServe(":8080", nil)
}

And here we go:

$ go run main.go &
[1] 31710
$ curl localhost:8080/metrics
dice_vertex_rolls_ 1 1538064026104
dice_vertex_throws_ 1 1538064026104
$ kill %1
$ go run main.go -use-report &
[2] 31798
[1]   Terminated              go run main.go
$ curl localhost:8080/metrics
# TYPE dice_vertex_rolls counter
dice_vertex_rolls 1 1538064037729

# TYPE dice_vertex_throws counter
dice_vertex_throws 1 1538064037729
yerden commented 6 years ago

oops, test is failing...

yerden commented 6 years ago

That's it. Tested some corner cases. Looks like the code behaves the same way with both API flavours now.

yerden commented 5 years ago

any news here?

achille-roussel commented 5 years ago

I was a bit concerned that this may alter the behavior of some other cases in subtle ways, but after doing a bit more testing I didn't notice any unexpected changes.

Thanks for the contribution @yerden