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.

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

feat: Internet Speed Monitor Input Plugin #9623

Merged
merged 12 commits into from
Sep 14, 2021
Merged

feat: Internet Speed Monitor Input Plugin #9623

merged 12 commits into from
Sep 14, 2021
+164 −2

Conversation

ersanyamarya
Copy link
Contributor

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.
  • Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

resolves #

Added an input plugin to monitor Internet Speed using golang library speedtest

@telegraf-tiger
Copy link
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger bot added 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 labels Aug 13, 2021
@telegraf-tiger
Copy link
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger
Copy link
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger
Copy link
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@ersanyamarya
Copy link
Contributor Author

!signed-cla

@ersanyamarya
Copy link
Contributor Author

Can a maintainer approve this pull request, please!

srebhan
srebhan requested changes Aug 23, 2021
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 for the cool PR @ersanyamarya! I have some comments in the code, but the following two things stick out:

  1. Please gather the speed without using the goroutine/channel construct you have now. Just simply call that function and let it return the speed and error to reduce unnecessary complexity.
  2. Please make sure that unit-tests requiring internet or network access are disabled in short testing mode.

Looking forward to the next review round...

@srebhan srebhan self-assigned this Aug 23, 2021
@ersanyamarya ersanyamarya requested a review from srebhan August 29, 2021 14:08
srebhan
srebhan reviewed Sep 7, 2021
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.

Hey @ersanyamarya thanks for the changes! Code looks quite good now, only a few (minor) comments left.

@ersanyamarya ersanyamarya requested a review from srebhan September 7, 2021 16:30
sspaink
sspaink approved these changes Sep 7, 2021
View reviewed changes
Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

@ersanyamarya This plugin looks good to me, thank you for resolving the requested changes! There seems to be a minor linter issue that will need to be resolved before this can be merged.

@srebhan Thanks for spending time reviewing this!

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Sep 7, 2021

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

srebhan
srebhan approved these changes Sep 8, 2021
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.

Looks perfect to me! Thanks @ersanyamarya for contributing!

@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 Sep 8, 2021
@ersanyamarya
Copy link
Contributor Author

Thank you @srebhan and @sspaink, looking forward to see me plugin in the release 📦 .

@srebhan
Copy link
Member

srebhan commented Sep 9, 2021

@ersanyamarya please note that it may take some time until you see the PR merged. That's normal and you should not get nervous. ;-)

@ersanyamarya
Copy link
Contributor Author

Thank you @srebhan for the information. I'm waiting to see my plugin in the release 👍🏼 .

@powersj powersj merged commit 40fa10b into influxdata:master Sep 14, 2021
@sjwang90
Copy link
Contributor

@ersanyamarya What instances does the plugin report latency? I've been testing this plugin and haven't been getting latency stats.

internet_speed,host=MBP15-SWANG.local download=109.59238503979421,upload=5.9275440400703046 1631741725000000000

@ersanyamarya
Copy link
Contributor Author

@ersanyamarya What instances does the plugin report latency? I've been testing this plugin and haven't been getting latency stats.

internet_speed,host=MBP15-SWANG.local download=109.59238503979421,upload=5.9275440400703046 1631741725000000000

Hey @sjwang90, Latency refers to the time taken by the speedTest servers for responding. If they don't have any latency it will not show up.

@sjwang90
Copy link
Contributor

Ah cool good to know. Thanks @ersanyamarya! Fun and easy configuration plugin.

@ersanyamarya
Copy link
Contributor Author

Ah cool good to know. Thanks @ersanyamarya! Fun and easy configuration plugin.

@sjwang90 , I am glad, the plugin is useful.

reimda pushed a commit that referenced this pull request Sep 17, 2021
@ersanyamarya @reimda
feat: Internet Speed Monitor Input Plugin (#9623)
353f579 
(cherry picked from commit 40fa10b)
@ersanyamarya ersanyamarya deleted the internetSpeedTest branch October 19, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sspaink sspaink sspaink approved these changes

@powersj powersj powersj approved these changes

@srebhan srebhan srebhan approved these changes

Assignees

@srebhan srebhan

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 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ersanyamarya @srebhan @sjwang90 @sspaink @powersj