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

For Prometheus Input add ability to query Consul Service catalog #5464

Merged
merged 1 commit into from
Jul 27, 2021
Merged

For Prometheus Input add ability to query Consul Service catalog #5464

merged 1 commit into from
Jul 27, 2021

Conversation

mmolnar
Copy link
Contributor

@mmolnar mmolnar commented Feb 21, 2019 •
edited
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

This PR adds the ability to query Consul Catalog for published prometheus metric endpoints.

Tests are not my strong suit, I will add them if this PR makes sense and there are no other issues with it.

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Feb 22, 2019
@danielnelson
Copy link
Contributor

Thanks the PR, this looks very interesting and useful. I'm really busy finalizing 1.10 up now so I'll have to take a closer look in a few weeks.

@danielnelson danielnelson added this to the 1.11.0 milestone Feb 22, 2019
@danielnelson danielnelson modified the milestones: 1.11.0, 1.12.0 May 24, 2019
@GeorgeMac GeorgeMac self-assigned this Jul 12, 2019
@danielnelson danielnelson modified the milestones: 1.12.0, 1.13.0 Aug 5, 2019
@sjwang90 sjwang90 modified the milestones: 1.13.0, 1.14.0 Nov 15, 2019
@sjwang90 sjwang90 modified the milestones: 1.14.0, 1.15.0 Feb 21, 2020
@danielnelson danielnelson removed this from the 1.15.0 milestone Jun 26, 2020
srebhan
srebhan reviewed Apr 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.

The code looks quite good already. I have some comments in the code, especially about using or rather not using log. Please also rebase the code to the current master!

@srebhan srebhan self-assigned this Apr 8, 2021
@srebhan
Copy link
Member

srebhan commented Apr 28, 2021

@mmolnar any chance you can work on this?

@mmolnar
Copy link
Contributor Author

mmolnar commented Apr 29, 2021

Thank you for the review. I will try to find time to work on it.

@srebhan
Copy link
Member

srebhan commented Jun 1, 2021

@mmolnar did you have time to take a look?

srebhan
srebhan reviewed Jun 13, 2021
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.

Almost perfect. :-) Thanks @mmolnar for keep this going! I added a comment regarding the potential "log-spam" issue and additionally found that currently refreshConsulServices() always returns nil as error. Can you please take a look there if we can come up with a refined "success" definition (see my comment in the code)?

srebhan
srebhan reviewed Jun 14, 2021
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.

Wow that was quick!
One cosmetic thing: can you please move the reset of lastQueryFailed out of the if clause. I'm always getting nervous when dealing with states, so it's better to involve the least number of branches possible... :-)

Oh and please check the linter-issues as we are trying hard to squash them all.

@srebhan
Copy link
Member

srebhan commented Jun 14, 2021 •
edited
Loading

This leaves the linter issues (see https://github.com/influxdata/telegraf/pull/5464/checks?check_run_id=2820187786)
You can check your branch locally running make lint-branch...

@mmolnar
Copy link
Contributor Author

mmolnar commented Jun 14, 2021

This leaves the linter issues (see https://github.com/influxdata/telegraf/pull/5464/checks?check_run_id=2820187786)
You can check your branch locally running make lint-branch...

Sorry for that, should be working correctly now.

@srebhan
Copy link
Member

srebhan commented Jun 14, 2021

!retry-failed

1 similar comment
@srebhan
Copy link
Member

srebhan commented Jun 14, 2021

!retry-failed

srebhan
srebhan approved these changes Jun 14, 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.

@mmolnar no worries. The code looks good to me. Thanks for keeping this going!

@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 Jun 14, 2021
akrantz01
akrantz01 suggested changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@akrantz01 akrantz01 left a comment

Choose a reason for hiding this comment

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

The code looks good to me. However, could you add the example config and some documentation to the README?

@mmolnar mmolnar closed this Jun 30, 2021
@mmolnar
Copy link
Contributor Author

mmolnar commented Jun 30, 2021

I must have messed something up while rebasing and pushing the requested changes to my fork...
It was not my intention to close this PR.

The changes are still available in the master branch of my fork.

@mmolnar mmolnar reopened this Jun 30, 2021
@telegraf-tiger
Copy link
Contributor

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

Artifact URLs

@mmolnar mmolnar requested a review from akrantz01 July 1, 2021 09:43
akrantz01
akrantz01 approved these changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@akrantz01 akrantz01 left a comment

Choose a reason for hiding this comment

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

LGTM!

helenosheaa
helenosheaa approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@helenosheaa helenosheaa 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, thanks for working on this!

@helenosheaa helenosheaa merged commit 1a42c7d into influxdata:master Jul 27, 2021
phemmer added a commit to phemmer/telegraf that referenced this pull request Aug 13, 2021
@phemmer
Merge remote-tracking branch 'origin/master' into postgres
fa5f937 
* origin/master: (183 commits)
  fix: CrateDB replace dots in tag keys with underscores (influxdata#9566)
  feat: Pull metrics from multiple AWS CloudWatch namespaces (influxdata#9386)
  fix: improve Clickhouse corner cases for empty recordset in aggregation queries, fix dictionaries behavior (influxdata#9401)
  fix(opcua): clean client on disconnect so that connect works cleanly (influxdata#9583)
  fix: Refactor ec2 init for config-api (influxdata#9576)
  fix: sort logs by timestamp before writing to Loki (influxdata#9571)
  fix: muting tests for udp_listener (influxdata#9578)
  fix: Do not return on disconnect to avoid breaking reconnect (influxdata#9524)
  fix: Fixing k8s nodes and pods parsing error (influxdata#9581)
  feat: OpenTelemetry output plugin (influxdata#9228)
  feat: Support AWS Web Identity Provider (influxdata#9411)
  fix: upgraded sensu/go to v2.9.0 (influxdata#9577)
  fix: Normalize unix socket path (influxdata#9554)
  docs: fix aws ec2 readme inconsistency (influxdata#9567)
  feat: Modbus Rtu over tcp enhancement (influxdata#9570)
  docs: information on new conventional commit format (influxdata#9573)
  docs: Add logo (influxdata#9574)
  docs: Adding links to net_irtt and dht_sensor external plugins (influxdata#9569)
  Upgrade hashicorp/consul/api to 1.9.1 (influxdata#9565)
  Update vmware/govmomi to v0.26.0 (influxdata#9552)
  Do not skip good quality nodes after a bad quality node is encountered (influxdata#9550)
  fix test so it hits a fake service (influxdata#9564)
  Update changelog
  Fix procstat plugin README to match sample config (influxdata#9553)
  Fix metrics reported as written but not actually written  (influxdata#9526)
  Prevent segfault in persistent volume claims (influxdata#9549)
  Update procstat to support cgroup globs & include systemd unit children (Copy of influxdata#7890) (influxdata#9488)
  Fix attempt to connect to an empty list of servers. (influxdata#9503)
  Fix handling bool in sql input plugin (influxdata#9540)
  Suricata alerts (influxdata#9322)
  Linter fixes for plugins/inputs/[fg]* (influxdata#9387)
  For Prometheus Input add ability to query Consul Service catalog (influxdata#5464)
  Support Landing page on Prometheus landing page (influxdata#8641)
  [Docs] Clarify tagging behavior (influxdata#9461)
  Change the timeout from all queries to per query (influxdata#9471)
  Attach the pod labels to the `kubernetes_pod_volume` & `kubernetes_pod_network` metrics. (influxdata#9438)
  feat(http_listener_v2): allows multiple paths and add path_tag (influxdata#9529)
  Bug Fix Snmp empty metric name (influxdata#9519)
  Worktable workfile stats (influxdata#8587)
  Update Go to v1.16.6 (influxdata#9542)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgeMac GeorgeMac GeorgeMac left review comments

@akrantz01 akrantz01 akrantz01 approved these changes

@srebhan srebhan srebhan approved these changes

@helenosheaa helenosheaa helenosheaa approved these changes

Assignees

@GeorgeMac GeorgeMac

@srebhan srebhan

Labels
area/consul area/prometheus feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin 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.
7 participants
@mmolnar @danielnelson @srebhan @GeorgeMac @akrantz01 @helenosheaa @sjwang90