Add influx2.0 output plugin #4645
Conversation
plugins/outputs/influxdb_v2/http.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("Bad value for 'Retry-After': %s", err.Error()) | ||
| } | ||
| time.Sleep(time.Second * time.Duration(retry)) |
There was a problem hiding this comment.
You may want a max sleep just in case the server says you sleep for a long time.
There was a problem hiding this comment.
10s seems reasonable to me as a max sleep. The big issue with the influxdb output is that, when under load, the next filled batch will almost immediately trigger the next write even after a write timeout.
There was a problem hiding this comment.
Oh but don't sleep and write because of #2919, hang on to the time and return. On the next call to Write check the time and if the amount of time has not passed then return an error.
Let's define a special error type specifically for this, something like CircuitOpenErr? We can put it in the internal package for now.
There was a problem hiding this comment.
Does the failed to write metric get cached, timestamp included? If the next call to write is less than the initial retry-time, would that second metric also get cached and be written on the next attempt that is after the retry-time? If the next write also returns a retry-time, is it acceptable to drop that new metric, even though it didn't get a chance to retry?
I put in a max retries limit based on the connection/httpClient, but it seemed wrong, would that be useful to put back in?
plugins/outputs/influxdb_v2/http.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("Bad value for 'Retry-After': %s", err.Error()) | ||
| } | ||
| time.Sleep(time.Second * time.Duration(retry)) |
There was a problem hiding this comment.
Oh but don't sleep and write because of #2919, hang on to the time and return. On the next call to Write check the time and if the amount of time has not passed then return an error.
Let's define a special error type specifically for this, something like CircuitOpenErr? We can put it in the internal package for now.
| } | ||
|
|
||
| i.serializer = influx.NewSerializer() | ||
| i.serializer.SetFieldTypeSupport(influx.UintSupport) |
There was a problem hiding this comment.
@goller do we want this set?
There was a problem hiding this comment.
@danielnelson , yes. We support uint.
There was a problem hiding this comment.
Is uint always desired? If someone imported non-uint data into the database, from say InfluxDB 1.6, this would be a change of type and could result in new data being uninsertable.
There was a problem hiding this comment.
Sounds like an issue for db migrations, right? This flag only allows the plugin to add uints if they are configured/used, if i understand correctly.
There was a problem hiding this comment.
Plus, with a major version change there are bound to have some incompatibilities. Keeping this always set seems like the best thing since the database supports uints from the getgo.
There was a problem hiding this comment.
I don't think a data migration could handle this since the database doesn't know what should be a int/uint. When it comes to Telegraf generated data, the only way to determine it today is by reading the code.
Unimplement non-important parts of the spec
plugins/outputs/influxdb_v2/http.go
Outdated
| retry = 0 | ||
| } | ||
| if retry > defaultMaxWait { | ||
| log.Println("E! [outputs.influxdb_v2] Failed to write metric: retry interval too long") |
There was a problem hiding this comment.
Make this more like:
if retry > defaultMaxWait {
retry = defaultMaxWait
}
|
|
||
| ## Content-Encoding for write request body, can be set to "gzip" to | ||
| ## compress body or "identity" to apply no encoding. | ||
| # content_encoding = "identity" |
There was a problem hiding this comment.
Let's change the default to gzip.
No description provided.