Add InfluxDB support for traefik metrics #2289
Conversation
|
Hi, Kindly requesting you guys to do a quick review before I cleanup the code and rebase. Thanks :) |
ldez
added
area/middleware/metrics
status/1-needs-design-review
and removed
status/0-needs-triage
labels
metrics/influx.go
Outdated
|
|
||
| import ( | ||
| "time" | ||
| "bytes" |
metrics/influx.go
Outdated
| influxdb "github.com/influxdata/influxdb/client/v2" | ||
| ) | ||
|
|
||
| var influxClient = influx.New(map[string]string{}, influxdb.BatchPointsConfig{}, kitlog.LoggerFunc(func(keyvals ...interface{}) error { |
There was a problem hiding this comment.
why kitlog? traefik use logrus
There was a problem hiding this comment.
This is dependent on go-kit/metrics/. Even other metrics related code uses the same
https://github.com/containous/traefik/blob/master/metrics/datadog.go#L9
adityacs
force-pushed
the
master
branch
2 times, most recently
from
bf7906b
to
2bdbd68
Compare
|
@adityacs could you run the tests locally?
https://github.com/containous/traefik/blob/master/CONTRIBUTING.md#tests |
|
@ldez I have added the unit tests and the code is working fine. Could you please review and approve this? |
adityacs
force-pushed
the
master
branch
2 times, most recently
from
39b4ddd
to
b52ea47
Compare
adityacs
force-pushed
the
master
branch
2 times, most recently
from
0811a28
to
221fadf
Compare
|
@adityacs We will review this PR, do not worry 😉 https://github.com/containous/traefik/blob/master/MAINTAINER.md#pr-review-process |
metrics/influx.go
Outdated
| kitlog "github.com/go-kit/kit/log" | ||
| "github.com/go-kit/kit/metrics/influx" | ||
| influxdb "github.com/influxdata/influxdb/client/v2" | ||
| "time" |
metrics/influx_test.go
Outdated
|
|
||
| import ( | ||
| "fmt" | ||
| "github.com/containous/traefik/types" |
metrics/influx.go
Outdated
|
|
||
| return &standardRegistry{ | ||
| enabled: true, | ||
| reqsCounter: influxClient.NewCounter(ddMetricsReqsName), |
There was a problem hiding this comment.
Could you please create specifics const for Influx instead of using DataDog const ?
const (
influxMetricsReqsName = "traefik.requests.total"
influxMetricsLatencyName = "traefik.request.duration"
influxRetriesTotalName = "traefik.backend.retries.total"
)
metrics/influx_test.go
Outdated
|
|
||
| type fn func() | ||
|
|
||
| func getMessage(t *testing.T, body fn) string { |
There was a problem hiding this comment.
I can see that you have duplicated some code from go-udp-testing. We are not able to accept your PR like that.
You have opened an issue on go-udp-testing. Could you try do open a PR please ?
There was a problem hiding this comment.
Related PR stvp/go-udp-testing#6
adityacs
force-pushed
the
master
branch
2 times, most recently
from
221fadf
to
a9afcfb
Compare
|
@mmatur Changes has been made. |
There was a problem hiding this comment.
Could you add documentation? https://github.com/containous/traefik/blob/master/docs/configuration/backends/web.md#metrics
Could you add the InfluxDB configuration?
https://github.com/containous/traefik/blob/d89b234cadfbf8e150d8a456b05bfb5f0526c5af/cmd/traefik/configuration.go#L54-L66
glide.lock
Outdated
| @@ -95,6 +95,8 @@ imports: | |||
| - name: github.com/coreos/etcd | |||
| version: c400d05d0aa73e21e431c16145e558d624098018 | |||
| subpackages: | |||
| - Godeps/_workspace/src/github.com/ugorji/go/codec | |||
| - Godeps/_workspace/src/golang.org/x/net/context | |||
metrics/influx_test.go
Outdated
| defer StopInflux() | ||
|
|
||
| if !influxRegistry.IsEnabled() { | ||
| t.Errorf("InfluxRegistry should return true for IsEnabled()") |
There was a problem hiding this comment.
t.Errorf -> t.Fatalf
And replace InfluxRegistry should return true for IsEnabled() by Influx registry must be enabled
metrics/influx_test.go
Outdated
| } | ||
|
|
||
| expected := []string{ | ||
| // We are only validating counts, as it is nearly impossible to validate latency, since it varies every run |
There was a problem hiding this comment.
Could you move the comment above?
metrics/influx_test.go
Outdated
|
|
||
| } | ||
|
|
||
| func extractAndMatchMessage(t *testing.T, expected []string, msg string) error { |
There was a problem hiding this comment.
Could you rename, change the signature and use t?
func assertMessage(t *testing.T, msg string, patterns []string) {
t.Helper()
for _, pattern := range patterns {
re := regexp.MustCompile(pattern)
match := re.FindStringSubmatch(msg)
if len(match) != 2 {
t.Errorf("Got %q %v, want %q", msg, match, pattern)
}
}
}Please don't take over code from others: https://github.com/go-kit/kit/blob/e2b298466b32c7cd5579a9b9b07e968fc9d9452c/metrics/influx/example_test.go#L98
metrics/influx.go
Outdated
| func initInfluxTicker(config *types.Influx) *time.Ticker { | ||
| address := config.Address | ||
| if len(address) == 0 { | ||
| address = "localhost:8089" |
There was a problem hiding this comment.
There was a problem hiding this comment.
http port default is 8086. We are creating a udp communication here. https://docs.influxdata.com/influxdb/v1.2/administration/config/#udp
metrics/influx.go
Outdated
|
|
||
| report := time.NewTicker(pushInterval) | ||
|
|
||
| var buf bytes.Buffer |
There was a problem hiding this comment.
Could you explain why this line is not in the go routine?
metrics/influx.go
Outdated
| return nil | ||
| })) | ||
|
|
||
| type bufWriter struct { |
There was a problem hiding this comment.
Could you explain why you created bufWriter?
There was a problem hiding this comment.
There was a problem hiding this comment.
We can call it with different name to avoid naming confusion
adityacs
force-pushed
the
master
branch
3 times, most recently
from
46cfd15
to
41cc131
Compare
|
@ldez Could you please validate the changes? |
|
@ldez @mmatur @emilevauge @bclermont Thanks a lot 😃 |
Add InfluxDB support for traefik metrics
This PR adds support for pushing traefik metrics to InfluxDB.