Skip to content
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: update the precision parameter default value #10814

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

powersj
Copy link
Contributor

@powersj powersj commented Mar 14, 2022

In #10803, precision parsing was made more strict so that incorrect
values would be caught. The default precision value however is not
correct as an empty string.

fixes: #10813

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Mar 14, 2022
@reimda
Copy link
Contributor

reimda commented Mar 14, 2022

I would guess that most telegraf users made their config files by running telegraf config or copying/editing /etc/telegraf/telegraf.conf that came in the deb/rpm. For all releases before this change, all those config files have precision = "", so it's a safe guess that a large portion, maybe almost all telegraf users have precision = "" in their telegraf.conf.

Making duration parsing more strict seems like a good thing, but it looks like that will mean requiring all those config files to be updated manually. I don't think that's practical.

I think we're going to need to add a special case in UnmarshalTOML to accept "" and treat it like telegraf did previously. Otherwise we're going to inconvenience too many people.

The original intent of the changes to UnmarshalTOML was to stop accepting a unit with no value (like "ns") because it's confusing/misleading. I think we can do that while still accepting "" for backward compatibility.

In influxdata#10803, precision parsing was made more strict so that incorrect
values would be caught. The default precision value however is not
correct as an empty string, but will still accept the emptry string.

fixes: influxdata#10813
@powersj powersj force-pushed the fix/precision-default branch from 516d6c5 to 9fa8210 Compare March 14, 2022 14:37
@powersj
Copy link
Contributor Author

powersj commented Mar 14, 2022

I would guess that most telegraf users made their config files by running telegraf config or copying/editing /etc/telegraf/telegraf.conf that came in the deb/rpm. For all releases before this change, all those config files have precision = "", so it's a safe guess that a large portion, maybe almost all telegraf users have precision = "" in their telegraf.conf.

Right, good catch

I think we're going to need to add a special case in UnmarshalTOML to accept "" and treat it like telegraf did previously. Otherwise we're going to inconvenience too many people.

Done - I am still updating the default value to align with the comment but checking for the empty string now.

Copy link

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! 🙂
Thanks also for looking out for the existing config files generated with telegraf config. 👍

@powersj powersj merged commit 8701ed1 into influxdata:master Mar 16, 2022
@powersj powersj deleted the fix/precision-default branch March 16, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using empty string for precision in AgentConfig causes startup error
4 participants