-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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(inputs.upsd): always convert to float #12516
Conversation
There are a number of values that can show up as a float or an int. This ensures the floats are always floats to avoid having some devices report as an int and others as a float and conflict in influxdb. Additionally, if we fail to convert in either the float or string case, then skip trying to add the field. fixes: influxdata#12501
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.
This looks good code-wise, but shouldn't we add a flag to switch between previous behavior and the new one? We could name it native_values
and discuss if the new or old behavior should be the default (I vote for the old one)...
unfortunately, yes. I do not like the situation it puts new users in where we know they could start to run into issue and have to turn on an option to get consistent data and then go fix their database. |
I'm not against setting the flag to a sensible default enabling the new behavior. We can still list this as a breaking change in the Changelog. |
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.
Looks good to me. One cosmetic suggestion though...
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 👍 This pull request doesn't change the Telegraf binary size 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Thanks @powersj!
(cherry picked from commit 4b445f6)
this still persists in the latest release
|
@sabey please open a new issue instead of commenting on an old PR! |
There are a number of values that can show up as a float or an int. This ensures the floats are always floats to avoid having some devices report as an int and others as a float and conflict in influxdb.
Additionally, if we fail to convert in either the float or string case, then skip trying to add the field.
fixes: #12501