Skip to content

[k8sprocessor] Add ability to associate metadata tags using pod UID rather than just IP #2199

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
Merged
merged 11 commits into from
Feb 25, 2021
Merged

[k8sprocessor] Add ability to associate metadata tags using pod UID rather than just IP #2199

merged 11 commits into from
Feb 25, 2021

Conversation

pmatyjasek-sumo
Copy link
Contributor

Description:
Add ability to associate metadata tags in k8s processor using pod UID rather than just IP

Link to tracking Issue: #1146

Testing:
Unit tests

Documentation:
In-code comments

@pmatyjasek-sumo pmatyjasek-sumo requested a review from a team January 27, 2021 14:23
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 27, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ Patryk Matyjasek (d90e3af2238249273579227b6f9c46a52ee15ae9)

Base automatically changed from master to main January 28, 2021 00:57
@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_k8s_pod_uid_labels_impl branch from d90e3af to 44c8f1e Compare January 28, 2021 12:45
@codecov
Copy link

codecov bot commented Jan 28, 2021 •
edited
Loading

Codecov Report

Merging #2199 (9721178) into main (63b2f33) will increase coverage by 0.00%.
The diff coverage is 99.06%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2199   +/-   ##
=======================================
  Coverage   90.45%   90.46%           
=======================================
  Files         397      397           
  Lines       19563    19614   +51     
=======================================
+ Hits        17695    17743   +48     
- Misses       1406     1408    +2     
- Partials      462      463    +1
Flag Coverage Δ
integration 69.37% <ø> (+0.06%) ⬆️
unit 89.25% <99.06%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...cessor/k8sprocessor/observability/observability.go 91.66% <50.00%> (-8.34%) ⬇️
processor/k8sprocessor/factory.go 97.36% <100.00%> (+0.03%) ⬆️
processor/k8sprocessor/kube/client.go 100.00% <100.00%> (ø)
processor/k8sprocessor/options.go 100.00% <100.00%> (ø)
processor/k8sprocessor/pod_association.go 100.00% <100.00%> (ø)
processor/k8sprocessor/processor.go 100.00% <100.00%> (ø)
receiver/k8sclusterreceiver/watcher.go 95.29% <0.00%> (-2.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63b2f33...9721178. Read the comment docs.

@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_k8s_pod_uid_labels_impl branch from 44c8f1e to 9725609 Compare January 28, 2021 13:07
jpkrohling
jpkrohling reviewed Jan 28, 2021
View reviewed changes
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Looks really nice!

processor/k8sprocessor/kube/client.go Outdated
return
}
}
c.Pods[pod.Status.PodIP] = newPod
Copy link
Member

Choose a reason for hiding this comment

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

Do I get it right that the same pod will exist twice in the list, once keyed by the IP and once by the ID?

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually discussed this with @pmatyjasek-sumo while he was working on the extension :) Having a single map and two references to the pod just makes the things easier, since this are different domains that have no risk of conflicts