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

pmatyjasek-sumo · 2021-01-27T14:23:05Z

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

linux-foundation-easycla · 2021-01-27T14:23:09Z

The committers are authorized under a signed CLA.

✅ Patryk Matyjasek (d90e3af2238249273579227b6f9c46a52ee15ae9)

codecov · 2021-01-28T12:59:33Z

Codecov Report

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

@@           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.

… rather than just IP Signed-off-by: Patryk Matyjasek <[email protected]>

jpkrohling

Looks really nice!

processor/k8sprocessor/config.go

processor/k8sprocessor/kube/client.go

jpkrohling · 2021-01-28T14:57:12Z

processor/k8sprocessor/kube/client.go

+					return
+				}
+			}
+			c.Pods[pod.Status.PodIP] = newPod


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?

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

@jpkrohling as @pmm-sumo mentioned we had some discussion on that and we decided that it should be ok if 2 keys from map will point to the same reference. With this we avoid to use two loops to iterate through two lists of Pods (one by IP and second by UID)

Sure, that's fine, but you might want to add the current size of this map in a metric somewhere: this is the kind of data that can easily explode in terms of number of elements. It's not unusual to have tens of thousands of pods in a given cluster, so, having this info would help people running the processor to identify any possible memory issue.

Was this done? If you don't to do it in this PR, create an issue to track this.

processor/k8sprocessor/pod_association.go

processor/k8sprocessor/doc.go

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

- Add more tests for added code - Refactor Association table - Remove unnecessary annonymous function

…static checks (#2199) Signed-off-by: Bogdan Drutu <[email protected]>

github-actions · 2021-02-06T05:25:51Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

jpkrohling · 2021-02-08T13:38:41Z

What's the current state of this PR? Is this ready to be reviews again?

pmatyjasek-sumo · 2021-02-08T14:32:39Z

@jpkrohling yes it is ready for review. I've addressed previous comments in latest commit.

jpkrohling · 2021-02-09T11:23:18Z

I have only one concern about the size the map might get, as it grows twice the rate of the number of pods being used. I believe the size of this map should be part of the set of metrics for the collector, as it would help correlate a possible increase in memory usage with the number of items in this map.

pmatyjasek-sumo · 2021-02-09T11:56:02Z

@jpkrohling that is a good idea, I'll put this metric soon

otelsvc/k8s/pod_table_size Signed-off-by: Patryk Matyjasek <[email protected]>

pmatyjasek-sumo · 2021-02-09T16:48:46Z

I've updated PR

jpkrohling

LGTM, just a minor comment about the metrics. If you don't want to work on them here, let me know and I'll approve the PR once the tracking issue is created.

jpkrohling · 2021-02-10T14:09:04Z

processor/k8sprocessor/observability/observability.go

+	mPodsUpdated  = stats.Int64("otelsvc/k8s/pod_updated", "Number of pod update events received", "1")
+	mPodsAdded    = stats.Int64("otelsvc/k8s/pod_added", "Number of pod add events received", "1")
+	mPodsDeleted  = stats.Int64("otelsvc/k8s/pod_deleted", "Number of pod delete events received", "1")
+	mPodTableSize = stats.Int64("otelsvc/k8s/pod_table_size", "Size of table containing pod info", "1")


Looks good, but is there a list of those metrics somewhere, telling users of this processor what they actually mean? Also, it would be a good opportunity to:

s/otelsvc/otelcol

replace the slashes by underscores, as it seems to be the standard at other metrics

If you don't want to do them here, feel free to open a tracking issue.

@jpkrohling I've created an issue and also I'll start work on it #2322

pmm-sumo · 2021-02-18T10:46:01Z

@tigrannajaryan this is the PR we discussed during Log SIG yesterday. It will allow us to tag logs with K8s metadata

owais

LGTM with a couple of minor comments.

My understanding is that if a user does not update config to specify the new pod association rules, the processor will continue to work with connection/attribute IP, right?

owais · 2021-02-18T10:54:53Z

processor/k8sprocessor/kube/client.go

+			id:   string(pod.UID),
 			name: pod.Name,
 			ts:   time.Now(),
 		})


nit: looks very repetitive. could be extracted into a function.

owais · 2021-02-18T11:25:06Z

processor/k8sprocessor/processor.go

-
-	// If IP is still not available by this point, nothing can be tagged here. Return.
-	if podIP == "" {
+	podAttributes := k8sPodAssociationFromAttributes(ctx, resource.Attributes(), kp.podAssociations)


We have another attrsToAdd := kp.getAttributesForPod() below and this one is named podAttributes as well. This could be a bit confusing to follow. IIRC, podAttributes here is actually a map of pod identifiers, such as IP, UID, etc, right? If so, let's name it podIdentifiers or something similar.

pmatyjasek-sumo · 2021-02-18T12:21:47Z

LGTM with a couple of minor comments.

My understanding is that if a user does not update config to specify the new pod association rules, the processor will continue to work with connection/attribute IP, right?

Yes it will work as previous

tigrannajaryan · 2021-02-18T14:35:12Z

processor/k8sprocessor/doc.go

-// If a match is found, the cached metadata is added to the data as resource attributes.
+// extracted metadata to the relevant spans, metrics and logs. The processor uses the kubernetes API to discover all pods
+// running in a cluster, keeps a record of their IP addresses, pod UIDs and interesting metadata.
+// The rules for associating the source record with specific Pod Metadata are configured via "pod_association" key.


"source record" is not an entirely clear term. Does this refer to the telemetry in pdata that is passing through the processor?

tigrannajaryan · 2021-02-18T14:37:32Z

processor/k8sprocessor/doc.go

+// It represents a list of rules that are executed in order until the first one is able to do the match.
+// Each rule is specified as a pair of from (representing the rule type) and name (representing the extracted key name).
+// Following rule types are available:
+//   from: "labels" - takes the attribute from resource attributes (the value can contain either IP address or Pod UID)


Does "resource" here refer to pdata.Resource or to the pod? Is this pdata.Resource attributes or pod labels? I am confused by mixed usage of "labels" and "attributes" here.

Ok I'll clarify that

processor/k8sprocessor/doc.go

* Add more descriptive in-code comments for PodAssociation, * Simplify PodAssociation logic * Update tests Signed-off-by: Patryk Matyjasek <[email protected]>

pmatyjasek-sumo · 2021-02-22T17:51:53Z

@tigrannajaryan I've addressed Your comments. Please check if it is clean now.

tigrannajaryan

Thank you @pmatyjasek-sumo
I think I understand the code a bit bitter, but would like a bit more clarifications if possible.

processor/k8sprocessor/processor.go

processor/k8sprocessor/pod_association.go

tigrannajaryan · 2021-02-23T00:34:17Z

processor/k8sprocessor/pod_association.go

+)
+
+// k8sPodAssociationFromAttributes extracts IP and pod UID from attributes
+func k8sPodAssociationFromAttributes(ctx context.Context, attrs pdata.AttributeMap, associations []kube.Association) map[string]string {


+1. Or alternatively remove that case from this function and handle it separately.

processor/k8sprocessor/pod_association.go

processor/k8sprocessor/client_test.go

processor/k8sprocessor/config.go

tigrannajaryan · 2021-02-23T01:03:25Z

processor/k8sprocessor/config.go

+	// Allowed values are connection and labels
+	From string `mapstructure:"from"`
+
+	// Name represents the name of the association.


In the answer below you wrote:

name (representing the extracted key name).

If "name" is extracted key name (which I think means the attribute name if I understand correctly) then what is "name of association"?

processor/k8sprocessor/processor.go

Co-authored-by: Tigran Najaryan <[email protected]>

* add custom type PodIdentifier * fix tags adding behavior * update comments * refactor pod association extracting Signed-off-by: Patryk Matyjasek <[email protected]>

pmatyjasek-sumo · 2021-02-23T11:25:27Z

@tigrannajaryan I've provided some refactor to IDs extraction and association metadata tags. I hope it is clean now

processor/k8sprocessor/doc.go

tigrannajaryan · 2021-02-23T23:20:04Z

processor/k8sprocessor/doc.go

+//  - from: connection
+//    name: ip
+//  - from: labels
+//    name: pod_uid


I may be wrong but if I understand correctly we cannot expect to see "pod_uid" as a Resource attribute in normal use cases. Let's use an attribute name that is realistic, even if it is just an example.

processor/k8sprocessor/pod_association.go

tigrannajaryan · 2021-02-23T23:25:06Z

processor/k8sprocessor/processor.go

-
-	// If IP is still not available by this point, nothing can be tagged here. Return.
-	if podIP == "" {
+	podIdentifierLabel, podIdentifierValue := extractPodIds(ctx, resource.Attributes(), kp.podAssociations)


I think it is best not to use the term "label" in this context. OpenTelemetry's vocabulary for Resources is "attribute name" or "attribute key". I think the following would be clear:

Suggested change

podIdentifierLabel, podIdentifierValue := extractPodIds(ctx, resource.Attributes(), kp.podAssociations)

podIdentifierKey, podIdentifierValue := extractPodIds(ctx, resource.Attributes(), kp.podAssociations)

processor/k8sprocessor/pod_association.go

tigrannajaryan · 2021-02-23T23:37:31Z

@tigrannajaryan I've provided some refactor to IDs extraction and association metadata tags. I hope it is clean now

@pmatyjasek-sumo yes, it looks much better, thanks. Please address the remaining comments.

* Additional comments, * Rename methods, * fix misspells Signed-off-by: Patryk Matyjasek <[email protected]>

pmatyjasek-sumo · 2021-02-25T15:46:34Z

@tigrannajaryan please take a look

tigrannajaryan

LGTM.
Thank you @pmatyjasek-sumo

…ather than just IP (#2199) 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

…rnal/tools (#2199) * Bump github.com/golangci/golangci-lint in /internal/tools Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.41.1 to 1.42.0. - [Release notes](https://github.com/golangci/golangci-lint/releases) - [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md) - [Commits](golangci/golangci-lint@v1.41.1...v1.42.0) --- updated-dependencies: - dependency-name: github.com/golangci/golangci-lint dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: MrAlias <[email protected]>

pmatyjasek-sumo requested review from dmitryax and owais as code owners January 27, 2021 14:23

pmatyjasek-sumo requested a review from a team January 27, 2021 14:23

github-actions bot assigned jpkrohling Jan 27, 2021

Base automatically changed from master to main January 28, 2021 00:57

pmatyjasek-sumo force-pushed the pm_k8s_pod_uid_labels_impl branch from d90e3af to 44c8f1e Compare January 28, 2021 12:45

Add ability to associate metadata tags in k8s processor using pod UID…

9725609

… rather than just IP Signed-off-by: Patryk Matyjasek <[email protected]>

pmatyjasek-sumo force-pushed the pm_k8s_pod_uid_labels_impl branch from 44c8f1e to 9725609 Compare January 28, 2021 13:07

jpkrohling reviewed Jan 28, 2021

View reviewed changes

pmatyjasek-sumo and others added 2 commits January 29, 2021 11:18

Apply suggestions from code review

0e3e203

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

Changes after review:

97d5e13

- Add more tests for added code - Refactor Association table - Remove unnecessary annonymous function

dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021

Move trace/span ids out of the proto-gen directory. Inlcude codecov, …

9fbd100

…static checks (#2199) Signed-off-by: Bogdan Drutu <[email protected]>

github-actions bot added the Stale label Feb 6, 2021

github-actions bot removed the Stale label Feb 9, 2021

Add metric to monitor pods map size.

c61adf4

otelsvc/k8s/pod_table_size Signed-off-by: Patryk Matyjasek <[email protected]>

jpkrohling reviewed Feb 10, 2021

View reviewed changes

pmatyjasek-sumo mentioned this pull request Feb 10, 2021

Metrics in k8s processor needs refactor #2322

Closed

jpkrohling approved these changes Feb 11, 2021

View reviewed changes

owais reviewed Feb 18, 2021

View reviewed changes

tigrannajaryan reviewed Feb 18, 2021

View reviewed changes

Address review comments.

e697580

* Add more descriptive in-code comments for PodAssociation, * Simplify PodAssociation logic * Update tests Signed-off-by: Patryk Matyjasek <[email protected]>

tigrannajaryan reviewed Feb 23, 2021

View reviewed changes

pmatyjasek-sumo and others added 2 commits February 23, 2021 08:54

Apply suggestions from code review

20547b9

Co-authored-by: Tigran Najaryan <[email protected]>

Code refactor:

8288315

* add custom type PodIdentifier * fix tags adding behavior * update comments * refactor pod association extracting Signed-off-by: Patryk Matyjasek <[email protected]>

tigrannajaryan reviewed Feb 23, 2021

View reviewed changes

Improve readability of the pod association code:

9721178

* Additional comments, * Rename methods, * fix misspells Signed-off-by: Patryk Matyjasek <[email protected]>

tigrannajaryan approved these changes Feb 25, 2021

View reviewed changes

tigrannajaryan merged commit bee7cb1 into open-telemetry:main Feb 25, 2021

	podIdentifierLabel, podIdentifierValue := extractPodIds(ctx, resource.Attributes(), kp.podAssociations)
	podIdentifierKey, podIdentifierValue := extractPodIds(ctx, resource.Attributes(), kp.podAssociations)

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

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

Uh oh!

Conversation

pmatyjasek-sumo commented Jan 27, 2021

Uh oh!

linux-foundation-easycla bot commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jpkrohling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 6, 2021

Uh oh!

jpkrohling commented Feb 8, 2021

Uh oh!

pmatyjasek-sumo commented Feb 8, 2021

Uh oh!

jpkrohling commented Feb 9, 2021

Uh oh!

pmatyjasek-sumo commented Feb 9, 2021

Uh oh!

pmatyjasek-sumo commented Feb 9, 2021

Uh oh!

jpkrohling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmm-sumo commented Feb 18, 2021

Uh oh!

owais left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmatyjasek-sumo commented Feb 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pmatyjasek-sumo commented Feb 22, 2021

Uh oh!

tigrannajaryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

edited

Loading

codecov bot commented Jan 28, 2021 •

edited

Loading