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(inputs.snmp_trap): Remove timeout deprecation #16145

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

DStrand1
Copy link
Member

@DStrand1 DStrand1 commented Nov 5, 2024

Summary

Removes the "unused" deprecation from the timeout field as it is still used

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #16137

@DStrand1 DStrand1 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 Nov 5, 2024
@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Nov 5, 2024
@DStrand1 DStrand1 removed their assignment Nov 5, 2024
@DStrand1 DStrand1 changed the title fix(inputs.snmp_trap): Only log timeout deprecation if set fix(inputs.snmp_trap): Remove timeout deprecation Nov 15, 2024
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.

@DStrand1 one comment. Furthemore, it might be good to add a comment to the setting in the sample.conf that this timeout is only used with the netsnmp translator.

plugins/inputs/snmp_trap/snmp_trap.go Outdated Show resolved Hide resolved
@Hipska
Copy link
Contributor

Hipska commented Nov 18, 2024

Looks like I deprecated it (partially) with #10339, but then it got re-added with #11534.

To me, it looks like the timeout should be removed completely, or set it fixed at 5s (a snmptranslate call should never take that long) I think it was added as a fail-save, since the call is run during runtime instead of with inputs.snmp it is only called on initialisation.

@srebhan
Copy link
Member

srebhan commented Nov 18, 2024

@Hipska it might be true that we should remove the timeout completely, but as it stands now the timeout is used and cannot be deprecated. If you want to remove it, please propose a holistic PR including the removal of the whole code path.

plugins/inputs/snmp_trap/snmp_trap.go Outdated Show resolved Hide resolved
plugins/inputs/snmp_trap/sample.conf Outdated Show resolved Hide resolved
plugins/inputs/snmp_trap/README.md Outdated Show resolved Hide resolved
@srebhan srebhan merged commit 9680123 into influxdata:master Nov 18, 2024
11 of 26 checks passed
@github-actions github-actions bot added this to the v1.32.3 milestone Nov 18, 2024
@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

⚠️ This pull request increases the Telegraf binary size by 2.31 % for linux amd64 (new size: 262.4 MB, nightly size 256.5 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

srebhan pushed a commit that referenced this pull request Nov 18, 2024
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 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.

DeprecationWarning: Option “timeout” of plugin inputs.snmp_trap
3 participants