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 selector tags for service (kube_inventory) #7267

Merged
merged 8 commits into from
Jun 18, 2020
Merged

Add selector tags for service (kube_inventory) #7267

merged 8 commits into from
Jun 18, 2020

Conversation

jimmyseto
Copy link
Contributor

@jimmyseto jimmyseto commented Apr 2, 2020 •
edited
Loading

Required for all PRs:

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

@jimmyseto
Copy link
Contributor Author

This is a small change to the kube_inventory plugin to extract selector information for a given service.

@jimmyseto
Copy link
Contributor Author

@danielnelson - this is a relatively small change. any chance we can get this low-hanging fruit reviewed and merged?

@jimmyseto jimmyseto force-pushed the add-selector-tags-for-service branch from bac2735 to ff8958d Compare April 29, 2020 19:10
@danielnelson
Copy link
Contributor

I'm concerned about this change producing a lot of new tags depending on how selectors are being used. Especially if certain bits of tooling are creating selectors automatically (perhaps Helm or Istio).

I think we should add a selector whitelist so that only the wanted selectors are added, we could use a style similar to what is done in the docker input plugin:

selectors_include = []
selectors_exclude = ["*"]

Another question is if this should be part of only the service type, or if we need similar behavior for all types?

@danielnelson danielnelson added area/k8s feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels May 6, 2020
@jimmyseto
add filter mechanism for selectors; add selector tags for other appli…
5a3bb22 
…cable k8s object types

- daemonset
- deployment
- persistentvolumeclaim
- pod
- statefulset
@jimmyseto
Copy link
Contributor Author

thanks for the feedback, @danielnelson

the potential influx of tags is a fair concern (similar to what we have with labels elsewhere). i think users can always configure their output plugin(s) to filter out undesired tags/selectors/labels, if needed. but, the ability to control this earlier at the input plugin level has its benefits. i've gone ahead, and implemented the filtering capability as you suggested.

i think selector information for the other types supported by kube_inventory is more of a nice-to-have. with the consolidated bits of information we can gather via the kubernetes plugin and prometheus plugin (kube-state-metrics), we can connect the dots between the various kubernetes object types today. i don't think we currently have this with kubernetes services (i.e. the ability to connect services to/from pods). either way, i've gone ahead, and added this for the other applicable object types as well.

@jimmyseto
Copy link
Contributor Author

hi @danielnelson - what do you think of the latest update? do they address your concerns? thanks.

@jimmyseto
Copy link
Contributor Author

@danielnelson - just wanted to make sure this was still on your radar. thanks in advance.

danielnelson
danielnelson reviewed Jun 17, 2020
View reviewed changes
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Just one minor thing and can you merge or rebase master since we merged a change that introduced some conflicts.

plugins/inputs/kube_inventory/kube_state.go Outdated
Comment on lines 128 to 131
//err = ki.createSelectorFilters()
//if err != nil {
// return err
//}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, and done.

danielnelson
danielnelson reviewed Jun 17, 2020
View reviewed changes
plugins/inputs/kube_inventory/kube_state.go Outdated
Comment on lines 73 to 77
## selectors to include and exclude as tags. Globs accepted.
## Note that an empty array for both will include all selectors as tags
## selector_exclude overrides selector_include if both set.
selector_include = []
selector_exclude = ["*"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, missed this. Let's initialize these options down in func init() { so that upgrades doesn't accidentally get set to include all. We can then list these commented out here in the sample configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@danielnelson danielnelson added this to the 1.15.0 milestone Jun 18, 2020
@danielnelson danielnelson merged commit c8b2423 into influxdata:master Jun 18, 2020
@danielnelson
Copy link
Contributor

Thanks!

@jimmyseto
Copy link
Contributor Author

Thanks!

thanks, @danielnelson. is there a roadmap/timeline for 1.15.0?

@danielnelson
Copy link
Contributor

Hoping to do RC1 tomorrow, then a release in about a week depending on what issues come up.

rhajek pushed a commit to bonitoo-io/telegraf that referenced this pull request Jul 13, 2020
@jimmyseto @rhajek
Add ability to add selectors as tags in kube_inventory (influxdata#7267)
0f52388
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
@jimmyseto
Add ability to add selectors as tags in kube_inventory (influxdata#7267)
4abca36
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@jimmyseto
Add ability to add selectors as tags in kube_inventory (influxdata#7267)
4c932d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielnelson danielnelson danielnelson left review comments

Assignees
No one assigned
Labels
area/k8s feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jimmyseto @danielnelson