Upgrade influxdb client to v2 #12
Conversation
|
// Empty Config
➜ go-playground go run main.go
2018/05/18 14:29:52 influxdb monitoring url not absolute url
exit status 1
// No Password
➜ go-playground go run main.go
2018/05/18 12:01:45 influxdb monitoring url https://aigle_dev@influxdb.theplant-dev.com:8086/aigle_dev not password
exit status 1
// No Database
➜ go-playground go run main.go
2018/05/18 12:01:51 influxdb monitoring url https://aigle_dev:PASSWORD@influxdb.theplant-dev.com:8086 not database
exit status 1// Worked
➜ go-playground go run main.go
level=info ts=2018-05-18T12:02:28.062421232+08:00 caller=influxdb.go:92 scheme=https username=aigle_dev database=aigle_dev host=influxdb.theplant-dev.com:8086 msg="influxdb instrumentation writing to https://aigle_dev@influxdb.theplant-dev.com:8086/aigle_dev" |
* Scheme is a must * Username/password isn't a must * Add unit tests for valid/invalid monitor urls
junhuif
force-pushed
the
monitoring-upgrade-influxdb-client
branch
from
2bf3d60 to
10e4026
Compare
As the CI & production is using go version 1.9.3. The original version is developed with go 1.10.2. But the `net/url` API has some different between the 1.9.3 and 1.10.2 which causing the test failure. * In go 1.10.2, no matter a url has userinfo or not, it's safe to call `u.User.UserName()` to get the username from the url. https://github.com/golang/go/blob/dev.boringcrypto.go1.10/src/net/url/url.go#L373-L379 ```go // Username returns the username. func (u *Userinfo) Username() string { if u == nil { return "" } return u.username } ``` * But in go 1.9.3, if there's no userinfo in the url, then calling `u.User.UserName()` will failure due to `u.User` is `nil`. https://github.com/golang/go/blob/dev.boringcrypto.go1.9/src/net/url/url.go#L368-L371 ```go // Username returns the username. func (u *Userinfo) Username() string { return u.username } ```
junhuif
force-pushed
the
monitoring-upgrade-influxdb-client
branch
from
10e4026 to
c5c9f85
Compare
There was a problem hiding this comment.
Generally fine, just a couple of minor updates needed. How much work do you think it would be to convert the monitor to a batching monitor?
At the moment, each data point is sent in a single request, right? So with lots of requests, the monitor will make lots of requests to InfluxDB.
This can be changed so that points are "batched" into a queue, and then a worker sends the queue to InfluxDB after P points, or after S seconds.
monitoring/influxdb.go
Outdated
| @@ -30,22 +31,46 @@ func NewInfluxdbMonitor(config InfluxMonitorConfig, logger log.Logger) (Monitor, | |||
| return nil, errors.Wrapf(err, "couldn't parse influxdb url %v", monitorURL) | |||
| } else if !u.IsAbs() { | |||
| return nil, errors.Errorf("influxdb monitoring url %v not absolute url", monitorURL) | |||
| } else if u.Scheme != "http" && u.Scheme != "https" { | |||
There was a problem hiding this comment.
This check already exists in the InfluxDB client:
monitoring/influxdb.go
Outdated
| "measurement", measurement, | ||
| "value", value, | ||
| "tags", tags, | ||
| "during", "influxdb.NewPoint", |
There was a problem hiding this comment.
Remember logger.With(...) for database, measurement, during.
As the checks already exists in the InfluxDB client: https://github.com/influxdata/influxdb/blob/531dc497177bd423b539db35cc0a48b36c4a280c/client/v2/client.go#L95
Maybe couple hours (< 1 day). (Have a talk with Azuma to get a basic of idea for doing this kind of things, thanks @azumads).
Right. |
|
since you have it fresh in your mind, can you work on it now please? |
Sorry, I found that I missed this message in that day. So I didn't do #13 yet. (Happy to do that when I'm free) |
Close #11.