Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics/influx: fix the package implementation #369

Merged
merged 5 commits into from
Oct 12, 2016
Merged

metrics/influx: fix the package implementation #369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac7fd0a
influxd metrics fixed
kpacha Oct 2, 2016
5198630
influx doc comment updated
kpacha Oct 10, 2016
c064ab5
execute the new influx tests as examples
kpacha Oct 10, 2016
e2b8274
examples moved to a dedicated file
kpacha Oct 11, 2016
121fac8
comments wrapped on 80col
kpacha Oct 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 27 additions & 20 deletions metrics/influx/influx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/go-kit/kit/log"
"github.com/go-kit/kit/metrics"
"github.com/go-kit/kit/metrics/generic"
"github.com/go-kit/kit/metrics/internal/lv"
)

Expand Down Expand Up @@ -108,10 +109,10 @@ func (in *Influx) WriteTo(w BatchPointsWriter) (err error) {
now := time.Now()

in.counters.Reset().Walk(func(name string, lvs lv.LabelValues, values []float64) bool {
fields := fieldsFrom(lvs)
fields["count"] = sum(values)
tags := mergeTags(in.tags, lvs)
var p *influxdb.Point
p, err = influxdb.NewPoint(name, in.tags, fields, now)
fields := map[string]interface{}{"count": sum(values)}
p, err = influxdb.NewPoint(name, tags, fields, now)
if err != nil {
return false
}
Expand All @@ -123,10 +124,10 @@ func (in *Influx) WriteTo(w BatchPointsWriter) (err error) {
}

in.gauges.Reset().Walk(func(name string, lvs lv.LabelValues, values []float64) bool {
fields := fieldsFrom(lvs)
fields["value"] = last(values)
tags := mergeTags(in.tags, lvs)
var p *influxdb.Point
p, err = influxdb.NewPoint(name, in.tags, fields, now)
fields := map[string]interface{}{"value": last(values)}
p, err = influxdb.NewPoint(name, tags, fields, now)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please update the comment on the Influx struct to reflect the new role of fields and tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Let's see what do you think about it...

BTW, very nice project!!!

if err != nil {
return false
}
Expand All @@ -138,16 +139,23 @@ func (in *Influx) WriteTo(w BatchPointsWriter) (err error) {
}

in.histograms.Reset().Walk(func(name string, lvs lv.LabelValues, values []float64) bool {
fields := fieldsFrom(lvs)
ps := make([]*influxdb.Point, len(values))
for i, v := range values {
fields["value"] = v // overwrite each time
ps[i], err = influxdb.NewPoint(name, in.tags, fields, now)
if err != nil {
return false
}
histogram := generic.NewHistogram(name, 50)
tags := mergeTags(in.tags, lvs)
var p *influxdb.Point
for _, v := range values {
histogram.Observe(v)
}
fields := map[string]interface{}{
"p50": histogram.Quantile(0.50),
"p90": histogram.Quantile(0.90),
"p95": histogram.Quantile(0.95),
"p99": histogram.Quantile(0.99),
}
p, err = influxdb.NewPoint(name, tags, fields, now)
if err != nil {
return false
}
bp.AddPoints(ps)
bp.AddPoint(p)
return true
})
if err != nil {
Expand All @@ -157,15 +165,14 @@ func (in *Influx) WriteTo(w BatchPointsWriter) (err error) {
return w.Write(bp)
}

func fieldsFrom(labelValues []string) map[string]interface{} {
func mergeTags(tags map[string]string, labelValues []string) map[string]string {
if len(labelValues)%2 != 0 {
panic("fieldsFrom received a labelValues with an odd number of strings")
panic("mergeTags received a labelValues with an odd number of strings")
}
fields := make(map[string]interface{}, len(labelValues)/2)
for i := 0; i < len(labelValues); i += 2 {
fields[labelValues[i]] = labelValues[i+1]
tags[labelValues[i]] = labelValues[i+1]
}
return fields
return tags
}

func sum(a []float64) float64 {
Expand Down
95 changes: 87 additions & 8 deletions metrics/influx/influx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
influxdb "github.com/influxdata/influxdb/client/v2"

"github.com/go-kit/kit/log"
"github.com/go-kit/kit/metrics/generic"
"github.com/go-kit/kit/metrics/teststat"
)

Expand All @@ -31,6 +30,28 @@ func TestCounter(t *testing.T) {
}
}

func TestCounter_multipleTags(t *testing.T) {
in := New(map[string]string{"a": "b"}, influxdb.BatchPointsConfig{}, log.NewNopLogger())
counter := in.NewCounter("influx_counter")
counter.Add(10)
counter.With("error", "true").Add(1)
counter.With("error", "false").Add(2)
counter.Add(50)

client := &bufWriter{}
in.WriteTo(client)

expectedLines := []string{
`influx_counter,a=b count=60 [0-9]+`,
`influx_counter,a=b,error=true count=1 [0-9]+`,
`influx_counter,a=b,error=false count=2 [0-9]+`,
}

if err := validateMessage(expectedLines, client.buf.String()); err != nil {
t.Error(err.Error())
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is now an Example, the test runner will take care of validating the output; there's no need for a separate step with the regexes. So,

in := New(...)
counter := in.NewCounter("...")
// etc.

client := &bufWriter{}
in.WriteTo(client)
io.Copy(os.Stdout, client.buf)

// Output:
// foo
// bar
// baz

should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the result is not deterministic because it adds a timestamp at the end of every line. That's why I used the regex extraction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why is the example not failing? Is it not being run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't fail because it extracts the timestamp before printing each line. so, instead of printing influx_counter,a=b count=60 1234567891234567890 (sample timestamp), it just prints influx_counter,a=b count=60.

If any of the given patterns doesn't match, it also prints an error message and the example fails

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! That's a bit tricky, but I understand now. Thanks.


func TestGauge(t *testing.T) {
in := New(map[string]string{"foo": "alpha"}, influxdb.BatchPointsConfig{}, log.NewNopLogger())
re := regexp.MustCompile(`influx_gauge,foo=alpha value=([0-9\.]+) [0-9]+`)
Expand All @@ -47,26 +68,73 @@ func TestGauge(t *testing.T) {
}
}

func TestGauge_multipleTags(t *testing.T) {
in := New(map[string]string{"a": "b"}, influxdb.BatchPointsConfig{}, log.NewNopLogger())
gauge := in.NewGauge("influx_gauge")
gauge.Set(10)
gauge.With("error", "true").Set(2)
gauge.With("error", "true").Set(1)
gauge.With("error", "false").Set(2)
gauge.Set(50)

client := &bufWriter{}
in.WriteTo(client)

expectedLines := []string{
`influx_gauge,a=b value=50 [0-9]+`,
`influx_gauge,a=b,error=true value=1 [0-9]+`,
`influx_gauge,a=b,error=false value=2 [0-9]+`,
}

if err := validateMessage(expectedLines, client.buf.String()); err != nil {
t.Error(err.Error())
}
}

func TestHistogram(t *testing.T) {
in := New(map[string]string{"foo": "alpha"}, influxdb.BatchPointsConfig{}, log.NewNopLogger())
re := regexp.MustCompile(`influx_histogram,foo=alpha bar="beta",value=([0-9\.]+) [0-9]+`)
re := regexp.MustCompile(`influx_histogram,bar=beta,foo=alpha p50=([0-9\.]+),p90=([0-9\.]+),p95=([0-9\.]+),p99=([0-9\.]+) [0-9]+`)
histogram := in.NewHistogram("influx_histogram").With("bar", "beta")
quantiles := func() (float64, float64, float64, float64) {
w := &bufWriter{}
in.WriteTo(w)
h := generic.NewHistogram("h", 50)
matches := re.FindAllStringSubmatch(w.buf.String(), -1)
for _, match := range matches {
f, _ := strconv.ParseFloat(match[1], 64)
h.Observe(f)
match := re.FindStringSubmatch(w.buf.String())
if len(match) != 5 {
t.Errorf("These are not the quantiles you're looking for: %v\n", match)
}
return h.Quantile(0.50), h.Quantile(0.90), h.Quantile(0.95), h.Quantile(0.99)
var result [4]float64
for i, q := range match[1:] {
result[i], _ = strconv.ParseFloat(q, 64)
}
return result[0], result[1], result[2], result[3]
}
if err := teststat.TestHistogram(histogram, quantiles, 0.01); err != nil {
t.Fatal(err)
}
}

func TestHistogram_multipleTags(t *testing.T) {
in := New(map[string]string{"foo": "alpha"}, influxdb.BatchPointsConfig{}, log.NewNopLogger())
histogram := in.NewHistogram("influx_histogram")
histogram.Observe(float64(10))
histogram.With("error", "true").Observe(float64(1))
histogram.With("error", "false").Observe(float64(2))
histogram.Observe(float64(50))

client := &bufWriter{}
in.WriteTo(client)

expectedLines := []string{
`influx_histogram,foo=alpha p50=10,p90=50,p95=50,p99=50 [0-9]+`,
`influx_histogram,error=true,foo=alpha p50=1,p90=1,p95=1,p99=1 [0-9]+`,
`influx_histogram,error=false,foo=alpha p50=2,p90=2,p95=2,p99=2 [0-9]+`,
}

if err := validateMessage(expectedLines, client.buf.String()); err != nil {
t.Error(err.Error())
}
}

func TestHistogramLabels(t *testing.T) {
in := New(map[string]string{}, influxdb.BatchPointsConfig{}, log.NewNopLogger())
h := in.NewHistogram("foo")
Expand All @@ -91,3 +159,14 @@ func (w *bufWriter) Write(bp influxdb.BatchPoints) error {
}
return nil
}

func validateMessage(expected []string, msg string) error {
for _, pattern := range expected {
re := regexp.MustCompile(pattern)
match := re.FindStringSubmatch(msg)
if len(match) != 1 {
return fmt.Errorf("Pattern not found! {%s} %s\n", pattern, msg)
}
}
return nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: above comments, I think this whole function can now be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the clarifications in the previous comment, this can stay :)