metrics/influx: fix the package implementation #369
Conversation
| 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) |
There was a problem hiding this comment.
Can you please update the comment on the Influx struct to reflect the new role of fields and tags?
There was a problem hiding this comment.
done.
Let's see what do you think about it...
BTW, very nice project!!!
Why? What is the advantage? |
|
aren't the examples automatically linked to the documentation? since those tests were added in order to give a better explanation about how the influx metrics work, I believe adding them in the doc will help a lot. |
| // may be mutated via With functions. Actual metric values are provided as | ||
| // Influx tags are attached to the Influx object, can be given to each | ||
| // metric at construction and can be updated anytime via With function. Influx fields | ||
| // are mapped to Go kit label values directly by this collector. Actual metric values are provided as | ||
| // fields with specific names depending on the metric. |
There was a problem hiding this comment.
Please wrap comments at 80col
| @@ -31,6 +30,33 @@ func TestCounter(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func ExampleCounter() { | |||
There was a problem hiding this comment.
Best not to mix Examples and real Tests in the same file. In the past I've stuck all the examples in an example_test.go file, can we do the same here?
There was a problem hiding this comment.
ok. I'll move them to the influxexample_test.go file
There was a problem hiding this comment.
Please just example_test.go in the appropriate directory.
| // influx_counter,a=b count=60 | ||
| // influx_counter,a=b,error=true count=1 | ||
| // influx_counter,a=b,error=false count=2 | ||
| } |
There was a problem hiding this comment.
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
// bazshould be sufficient.
There was a problem hiding this comment.
the result is not deterministic because it adds a timestamp at the end of every line. That's why I used the regex extraction.
There was a problem hiding this comment.
Then why is the example not failing? Is it not being run?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah! That's a bit tricky, but I understand now. Thanks.
| fmt.Println(match[1]) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
re: above comments, I think this whole function can now be removed.
There was a problem hiding this comment.
With the clarifications in the previous comment, this can stay :)
|
The branch now is including the changes you requested:
Let me know if there is something else we could improve |
|
Nice! Thank you very much for the contribution. |
metrics/influx: fix the package implementation
As reviewing the #367 I've found all the implementations of the metrics used the labels as fields, so I generalized the hack for gauges and histograms too.
Also, the histogram implementation wasn't aggregating the results, so they couldn't be stored properly in the backend (same problem as in #367, because influx accepts just one combination of fields for a given combination of tags and timestamp).
Finally, I added 3 tests where you can see how the messages are being sent over the wire.