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

Add HTTP proxy setting to New Relic output plugin #8749

Merged
merged 6 commits into from
Feb 1, 2021
Merged

Add HTTP proxy setting to New Relic output plugin #8749

merged 6 commits into from
Feb 1, 2021

Conversation

msummers-nr
Copy link
Contributor

@msummers-nr msummers-nr commented Jan 26, 2021 •
edited
Loading

Required for all PRs:

  • [x ] Associated README.md updated.
  • [x ] Has appropriate unit tests.

Update New Relic output plugin:

  • Add http_proxy as a configuration value override

telegraf-tiger[bot]
telegraf-tiger bot approved these changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

🤝 ✅ CLA has been signed. Thank you!

srebhan
srebhan requested changes Jan 27, 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 this PR! To get this merged you have to

  1. Get rid of all TODOs in the code!
  2. Check the errors returned by getHTTPClient() or change the code structure! See my comments in the code.

There are also some minor comments in the code. Furthermore it would be nice to change the PR title to something reflecting your change e.g. "Add HTTP proxy setting to New Relic output plugin.".

@srebhan srebhan self-assigned this Jan 27, 2021
@srebhan srebhan added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Jan 27, 2021
@msummers-nr msummers-nr changed the title Update New Relic output plugin Add HTTP proxy setting to New Relic output plugin, transmit string values as tags Jan 27, 2021
srebhan
srebhan requested changes Jan 27, 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.

Nice. The only thing is the change in behavior for string values. To keep the output of the plugin stable, please add an option to allow to keep the current behavior by default and opt-in for the new behavior (string values as tags). Comment in the code.

@msummers-nr msummers-nr changed the title Add HTTP proxy setting to New Relic output plugin, transmit string values as tags Add HTTP proxy setting to New Relic output plugin Jan 27, 2021
srebhan
srebhan approved these changes Jan 27, 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 good to me.

@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 Jan 27, 2021
@msummers-nr
Copy link
Contributor Author

Looks good to me.

@srebhan Thanks for your help

@ssoroka ssoroka merged commit c43de16 into influxdata:master Feb 1, 2021
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@msummers-nr
Add HTTP proxy setting to New Relic output plugin (influxdata#8749)
d47b487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssoroka ssoroka ssoroka approved these changes

@srebhan srebhan srebhan approved these changes

@ivorybilled ivorybilled ivorybilled approved these changes

@telegraf-tiger telegraf-tiger[bot] telegraf-tiger[bot] approved these changes

Assignees

@srebhan srebhan

Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out 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.
4 participants
@msummers-nr @ssoroka @srebhan @ivorybilled