Skip to content

feat(inputs.fritzbox): Add plugin #16390

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Mar 18, 2025
Merged

feat(inputs.fritzbox): Add plugin #16390

merged 25 commits into from
Mar 18, 2025

Conversation

hdecarne
Copy link
Contributor

@hdecarne hdecarne commented Jan 10, 2025 •
edited
Loading

Summary

This PR provides the inputs.fritzbox plugin for collecting statistics from AVM devices (routers, repeaters, ...). It uses the device's TR-064 interfaces to retrieve the status.

I previously contributed this plugin as an external one. As part of the latest reworks I also converted it into an internal one.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #16389

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Jan 10, 2025
@hdecarne hdecarne changed the title feat(inputs.fritzbox): Initial implementation feat(inputs.fritzbox): Make inputs.fritzbox an internal plugin Jan 10, 2025
Hipska
Hipska suggested changes Jan 15, 2025
View reviewed changes
@hdecarne
Copy link
Contributor Author

@Hipska I have reworked the plugin accordingly (see comments/commits for details). Please let me know if there are additional issues.

Hipska
Hipska suggested changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Added another round

@hdecarne
Copy link
Contributor Author

@Hipska, thanks for the detailed review. I have reworked the plugin's configuration interface overall as suggested.

Hipska
Hipska reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Great progress! Only some minor comments/questions this time.

Hipska
Hipska suggested changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

All good, just a small correction needed around the wait groups, see this example: https://gobyexample.com/waitgroups

@srebhan srebhan changed the title feat(inputs.fritzbox): Make inputs.fritzbox an internal plugin feat(inputs.fritzbox): Add plugin Mar 3, 2025
srebhan
srebhan reviewed Mar 3, 2025
View reviewed changes
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 @hdecarne! Some comments, mostly similar to those for the other plugins...

@srebhan srebhan self-assigned this Mar 3, 2025
@hdecarne
Copy link
Contributor Author

hdecarne commented Mar 4, 2025

@srebhan, @Hipska, I took care of the latest comments.
One question regarding testutil.RequireMetricsEqual . How to deal with ValueType not equal Untyped? Is there a way to exclude this from comparison?
For the testcase based approach now, this is simply set to the found ValueType, as I wanted to avoid the unreasonable effort to make this testcase dependendent.

@srebhan
Copy link
Member

srebhan commented Mar 6, 2025

@hdecarne PR #16493 will add a testutil.IgnoreType function which allows exactly that, to ignore the type... We are trying to push it ASAP and then you can rebase on master (and fix the merge conflicts) to use it. I'll let you know once this is in...

@srebhan
Copy link
Member

srebhan commented Mar 7, 2025

@hdecarne PR #16493 is merged so you now can use the IgnoreType function after rebasing...

srebhan
srebhan reviewed Mar 7, 2025
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @hdecarne for the update. Some more comments, especially for testing. Please try to use a structure and naming similar to those in other plugins as it will ease review and navigating the code later.

Hipska
Hipska reviewed Mar 10, 2025
View reviewed changes
srebhan
srebhan approved these changes Mar 14, 2025
View reviewed changes
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 a lot @hdecarne for your contribution!

@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 14, 2025
@srebhan srebhan assigned skartikey and mstrandboge and unassigned srebhan Mar 14, 2025
skartikey
skartikey reviewed Mar 17, 2025
View reviewed changes
skartikey
skartikey approved these changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@skartikey skartikey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@hdecarne Thanks for your contribution! This looks good to me. 👍

@skartikey skartikey removed 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 18, 2025
@skartikey skartikey merged commit 1406f41 into influxdata:master Mar 18, 2025
5 checks passed
@github-actions github-actions bot added this to the v1.35.0 milestone Mar 18, 2025
@hdecarne hdecarne deleted the fritzbox-plugin branch March 23, 2025 05:06
justinwwhuang pushed a commit to justinwwhuang/telegraf_fork that referenced this pull request Mar 24, 2025
@hdecarne @justinwwhuang
feat(inputs.fritzbox): Add plugin (influxdata#16390)
8a5c0a5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skartikey skartikey skartikey approved these changes

@srebhan srebhan srebhan approved these changes

+1 more reviewer

@Hipska Hipska Hipska approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@skartikey skartikey

Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins
Projects
None yet
Milestone
v1.35.0
Development

Successfully merging this pull request may close these issues.

Convert external plugin inputs.fritzbox into an internal one
5 participants
@hdecarne @srebhan @Hipska @skartikey @mstrandboge