Skip to content

fix(inputs.ping): Warn on using timeout parameter for native method#18455

Merged
skartikey merged 3 commits intoinfluxdata:masterfrom
WZH8898:fix-ping-native-timeout
Mar 10, 2026
Merged

fix(inputs.ping): Warn on using timeout parameter for native method#18455
skartikey merged 3 commits intoinfluxdata:masterfrom
WZH8898:fix-ping-native-timeout

Conversation

@WZH8898
Copy link
Contributor

@WZH8898 WZH8898 commented Mar 4, 2026

Summary

Warn when the timeout parameter is set with method = "native", since the native pinger ignores per-ping timeouts.

Updated documentation and sample config to clarify that deadline should be used to control timeout behavior for the native method.

Checklist

Related issues

resolves #18395

@telegraf-tiger telegraf-tiger bot added area/ping fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Mar 4, 2026
@WZH8898 WZH8898 force-pushed the fix-ping-native-timeout branch from d00b3b1 to 8952c84 Compare March 4, 2026 09:08
@srebhan
Copy link
Member

srebhan commented Mar 4, 2026

@WZH8898 please restore the PR description template, especially the AI section, so we can review your contribution!

@srebhan srebhan self-assigned this Mar 4, 2026
@WZH8898
Copy link
Contributor Author

WZH8898 commented Mar 4, 2026

@WZH8898 please restore the PR description template, especially the AI section, so we can review your contribution!

Thanks! I've restored the PR template and added the AI checklist section.
Please let me know if anything else needs to be adjusted.

@srebhan
Copy link
Member

srebhan commented Mar 5, 2026

@WZH8898 to be honest I don't like the approach too much as it has too many assumptions. How about rather than trying to guess ask the user to use the deadline configuration option instead as this is what is used already? I.e. if you see timeout to be set with the native method, you should error out (or warn) in Init() that this parameter is not used for the method and what to do instead. Maybe also add some documentation to the README?

@WZH8898 WZH8898 force-pushed the fix-ping-native-timeout branch from 8952c84 to dc17cd5 Compare March 6, 2026 01:21
@WZH8898
Copy link
Contributor Author

WZH8898 commented Mar 6, 2026

@WZH8898 to be honest I don't like the approach too much as it has too many assumptions. How about rather than trying to guess ask the user to use the deadline configuration option instead as this is what is used already? I.e. if you see timeout to be set with the native method, you should error out (or warn) in Init() that this parameter is not used for the method and what to do instead. Maybe also add some documentation to the README?

Thanks for the suggestion! I've updated the implementation to warn when timeout is used with the native method and documented the behavior in the README.
Please let me know if further adjustments are needed.

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks @WZH8898! Some more small comments...

Comment on lines +93 to +95
if p.Method == "native" && p.Timeout > 0 && p.Log != nil {
p.Log.Warn(`"timeout" is ignored when method = "native"; use "deadline" to control the total runtime`)
}
Copy link
Member

Choose a reason for hiding this comment

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

Log always is non-nil at runtime. If you see a panic here it's because the tests do not define Log which is a bug and should be fixed by setting the logger in the test!

@srebhan srebhan changed the title fix(inputs.ping): honor timeout parameter for native method fix(inputs.ping): Warn on using timeout parameter for native method Mar 6, 2026
@WZH8898 WZH8898 force-pushed the fix-ping-native-timeout branch from dc17cd5 to a12fb43 Compare March 9, 2026 01:43
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@WZH8898 WZH8898 force-pushed the fix-ping-native-timeout branch from a2f515c to 12e1177 Compare March 9, 2026 02:55
@WZH8898
Copy link
Contributor Author

WZH8898 commented Mar 9, 2026

Thanks @WZH8898! Some more small comments...

Thanks for the suggestions! I've updated the code and applied the suggested changes.

@WZH8898 WZH8898 force-pushed the fix-ping-native-timeout branch 2 times, most recently from c6d5cb6 to 86c0281 Compare March 9, 2026 10:31
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@WZH8898 WZH8898 force-pushed the fix-ping-native-timeout branch from 86c0281 to b1e1e50 Compare March 9, 2026 10:43
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Mar 9, 2026

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you very much @WZH8898!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Mar 10, 2026
@srebhan srebhan assigned skartikey and mstrandboge and unassigned srebhan Mar 10, 2026
Copy link
Contributor

@skartikey skartikey left a comment

Choose a reason for hiding this comment

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

@WZH8898 Thanks for the contribution!

@skartikey skartikey merged commit 52a1eef into influxdata:master Mar 10, 2026
29 checks passed
@github-actions github-actions bot added this to the v1.38.1 milestone Mar 10, 2026
@Hipska
Copy link
Contributor

Hipska commented Mar 13, 2026

This PR now adds this warning even when not setting a timeout when using native method..

srebhan added a commit that referenced this pull request Mar 16, 2026
…18455)

Co-authored-by: wanzh <wanzh@nscc-tj.cn>
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
(cherry picked from commit 52a1eef)
@Hipska
Copy link
Contributor

Hipska commented Mar 17, 2026

Bummer this got released 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ping fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inputs.ping with method "native" ignores "timeout" parameter

5 participants