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

consul input - allow flags to ignore very high cardinality metrics #7643

Closed
wants to merge 5 commits into from

Conversation

chuckyz
Copy link

@chuckyz chuckyz commented Jun 6, 2020

Required for all PRs:

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

Comment on lines 48 to 51
## Disable gathering check id from Consul on health checks
# This is useful in dynamic environments, where check_id is generated,
# where most check_id's are some uuid-ish name with low meaning.
# disable_check_id = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this option, I suggest using tagexclude = ["check_id"]. This is a general solution that can be used on any plugin.

Copy link
Author

@chuckyz chuckyz Jun 6, 2020

Choose a reason for hiding this comment

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

Sure, do you mind if I leave a note in the readme for this suggesting that for our (and likely others) scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good

Comment on lines 42 to 46
## Disable gathering tags from Consul on health checks
# This is very useful on large clusters with a lot of services and tags.
# This alleviates the situation of this telegraf running on multiple servers
# for a holistic point of view, and there are thousands of checks with 10+ tags.
# disable_tags = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think you could update this to match the method I suggested here: #7387 (comment)

You can set this up fairly easily by using filters.NewIncludeExcludeFilter.

Copy link
Author

@chuckyz chuckyz Jun 6, 2020

Choose a reason for hiding this comment

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

Sure! So we're on on the same page, to replicate the behaviour I want, I'd need to set:

[[inputs.consul]]
  ... # config
  service_tag_include = []
  service_tag_exclude = ["*"]

The behaviour I'm expecting would be for this is for the input to pass along service_id, node, and check_id, then check_id can be excluded with tagexclude = ["check_id"] to replicate what I've tried here.

Is my understanding correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's right. To clarify the behavior of the service_tag_include and service_tag_exclude options, they would match the entire ServiceTag string, before any splitting, but would only affect these tags.

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 6, 2020
@chuckyz
Copy link
Author

chuckyz commented Jun 10, 2020

@danielnelson can you re-review? I mostly based this off of what was in ECS and the Docker inputs. I also moved the client + filter initialization into it's own function called by Gather, to allow the test suite to call the same function to initialize the filters.

@chuckyz chuckyz requested a review from danielnelson June 11, 2020 00:59
@reimda reimda removed the request for review from danielnelson June 17, 2021 21:02
@srebhan
Copy link
Member

srebhan commented May 30, 2022

@chuckyz can you please rebase this PR and resolve the merge conflicts?

@srebhan srebhan self-assigned this May 30, 2022
@srebhan
Copy link
Member

srebhan commented Jun 22, 2022

I'll close this due to inactivity. If you are still interested in getting this merged, please reopen this PR or drop me a note.

@srebhan srebhan closed this Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/consul feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants