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

Kubernetes processor enhancements #4068

Merged
merged 1 commit into from
Apr 25, 2017

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Apr 20, 2017

Add several small enhancements and fixes to kubernetes processor (WIP):

  • Move it to the root folder (_meta lookups expect that)
  • Fix podwatcher error handling
  • Flag it as beta
  • Fix config validation for in_cluster use
  • Move processor fields to a common kubernetes namespace

Related to #4027

@exekias exekias added enhancement in progress Pull request is currently in progress. libbeat labels Apr 20, 2017
@exekias exekias force-pushed the k8s-processor-enhancements branch from 1cb827e to c9ef182 Compare April 20, 2017 11:06
@exekias exekias force-pushed the k8s-processor-enhancements branch 3 times, most recently from 6795072 to 24fbb1c Compare April 24, 2017 14:35
@exekias exekias added review and removed in progress Pull request is currently in progress. labels Apr 24, 2017
@exekias
Copy link
Contributor Author

exekias commented Apr 24, 2017

should by ready!

@exekias exekias force-pushed the k8s-processor-enhancements branch from 24fbb1c to 79a2c9d Compare April 24, 2017 15:33
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Nice to have it all under one namespace. Left a minor comment. We can also merge it as is and address it in a follow up PR.

description: >
Kubernetes namespace

- name: kubernetes.labels
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be singular

description: >
Kubernetes labels map

- name: kubernetes.annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

This shoud be singular. Reason if someone is going to "read it" it will be kubernetes.label.testlabel=18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been reading https://www.elastic.co/guide/en/beats/libbeat/5.3/event-conventions.html and also saw how we do it in docker module: https://github.com/elastic/beats/blob/master/metricbeat/module/docker/container/_meta/data.json#L13

It sounds more natural to me to have labels as plural. It's like you are naturally selecting a label by name from the labels dict, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

labels sounds better to me too, but I am fine also with singular.

 * Move kubernetes processor to root processors folder
 * Restart watch call on error
 * Flag kubernetes proccesor as beta
 * Fix config validation for `in_cluster` cases
 * Add kubernetes metadata fields
 * Put kubernetes processor fields under a common namespace
@exekias exekias force-pushed the k8s-processor-enhancements branch from 79a2c9d to 0808b39 Compare April 25, 2017 06:54
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

WFG

@ruflin
Copy link
Contributor

ruflin commented Apr 25, 2017

jenkins, retest it

@ruflin ruflin merged commit 5afda3d into elastic:master Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants