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

K8S: Adding ignored pod names as config parameter #3520

Merged
merged 17 commits into from
Jun 16, 2021

Conversation

euniceek
Copy link
Contributor

Description:
This PR addresses #1077 by adding a new parameter to the K8S configuration allowing users to define names of pods to ignore while tagging. Previously, the names are hardcoded and noted as TODO.

Testing:

  • make was used to test the new implementation and to ensure no other pre-existing functionality was broken.

  • New unit tests were added to ensure that the default ignored_pod_names are [jaeger-agent, jaeger-collector] and that users will be able to add additional pod names.

@euniceek euniceek requested a review from a team May 25, 2021 00:23
@bogdandrutu
Copy link
Member

/cc @owais @jrcamp


ignored_pod_names:
- jaeger-agent
- jaeger-collector
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we already have filter config above, I think adding a more generic exclude would be better than something dedicated to just ignoring pods. May be something like

exclude:
    pods:
        - jaeger-agent
        - jaeger-collector

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think the following would be even better as it's more future proof. This can allow us to exclude pods by fields other than name in future if we want.

exclude:
    pods:
        - name: jaeger-agent
        - name: jaeger-collector

Copy link
Contributor Author

@euniceek euniceek May 25, 2021

Choose a reason for hiding this comment

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

Sounds good. Made those changes! :)

Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

Accidentally approved :)

@euniceek euniceek force-pushed the k8s-config-pod-names branch 4 times, most recently from 93003ce to f0c8740 Compare May 26, 2021 05:54
@euniceek euniceek requested a review from owais May 26, 2021 16:10
processor/k8sprocessor/config.go Outdated Show resolved Hide resolved
processor/k8sprocessor/config.go Outdated Show resolved Hide resolved
processor/k8sprocessor/kube/client.go Outdated Show resolved Hide resolved
processor/k8sprocessor/kube/kube.go Show resolved Hide resolved
processor/k8sprocessor/kube/kube.go Outdated Show resolved Hide resolved
// Check well known names that should be ignored
for _, rexp := range podNameIgnorePatterns {
// Check if user requested the pod to be ignored through configuration
for _, rexp := range c.Exclude.Regex {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we make the changes suggested below, this should read something like the following:

for _, excludedPod := range c.Exclude.Pods {
 		if excludedPod.Name.MatchString(pod.Name) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback and help! The requested changes have been made.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

PR looks good to me but CI is failing because of an unrelated failure (probably you need to rebase/merge main to get #3735?). Could you try to fix the issue?

@mx-psi
Copy link
Member

mx-psi commented Jun 16, 2021

I am going to add the ready-to-merge label since this looks ready and has three approvals

@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Jun 16, 2021
@bogdandrutu bogdandrutu merged commit 5ccdbe0 into open-telemetry:main Jun 16, 2021
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
On error pdata.Metrics may not be initialized so guard against it. Update test
to return uninitialized object on error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants