-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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.
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
fix(outputs.redistimeseries): Handle string fields correctly #14060
fix(outputs.redistimeseries): Handle string fields correctly #14060
Conversation
Well, that's a great idea. There are some other outputs that also don't support strings which could benefit from this. A lot of them are just simply dropping any string, regardless of it's value. |
@Hipska feel free to reuse the code! |
Maybe I should list these on a new Issue?
And probably many more.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little concerned about the additional logging during field handling. This might reveal some new "issues" to end-users that are rather verbose. However, this handling is the correct change.
2239ffa
to
21ee760
Compare
I've rebased on master to resolve the conflicts |
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
(cherry picked from commit 68eda25)
resolves #14058
Redis Timeseries do only support numeric field values. Currently, we bluntly send the values to Redis irrespective of their type and receive errors on string fields that convert non-numeric characters. This error then aborts the write and in turn to a retry which will only retrigger the error.
This PR adds a
convert_string_fields
option which by default tries to convert string-fields to numeric values succeeding if they e.g. contain"3.14"
or"0"
but failing on any non-convertible values like"3.1.2"
or"foo"
. In the former case the value is sent to Redis while in the latter (failing) case the field is dropped but sending is continued. This resembles the current behavior as close as possible.When setting the option to
false
all string fields are dropped and no attempt is made to safe them.In the course of the PR, we also update the redis client to v9 allowing support for timeseries commands and adding unit tests including a "testcases" framework.