[processor/k8sattribute] support attribute k8s.deployment.uid #14003

fatsheep9146 · 2022-09-09T05:49:26Z

Signed-off-by: Ziqi Zhao zhaoziqi9146@gmail.com

fix #12936

fatsheep9146 · 2022-09-14T04:39:15Z

@dmitryax The pr is ready for review.

fatsheep9146 · 2022-09-26T04:49:54Z

ping @dmitryax, could you help review this?

povilasv

I think maybe instead of going this route, we should discover ReplicaSets and get rid of Deployment regex?

The idea behind this, is that regex can match things that are not managed by Deployment, and we would still set Deployment name incorrectly.

So I think we should instead discover ReplicaSets and parse the Pod's Owner References to find the ReplicaSet. Example Pod Owner references:

apiVersion: v1
kind: Pod
metadata:
  labels:
    app.kubernetes.io/instance: opentelemetry-collector
    app.kubernetes.io/name: opentelemetry-collector
    component: standalone-collector
    pod-template-hash: 6c45f8d6f6
  name: opentelemetry-collector-6c45f8d6f6-s4jq4
  ownerReferences:                                                  
  - apiVersion: apps/v1
    blockOwnerDeletion: true
    controller: true 
    kind: ReplicaSet
    name: opentelemetry-collector-6c45f8d6f6
    uid: ee75293d-14ec-42a0-9548-a768d9e07c48

And from ReplicaSet's Owner Reference we can get Deployment name, and id:

apiVersion: apps/v1
kind: ReplicaSet
metadata:
 labels:
   app.kubernetes.io/instance: opentelemetry-collector
   app.kubernetes.io/name: opentelemetry-collector
   component: standalone-collector
   pod-template-hash: 6c45f8d6f6
 name: opentelemetry-collector-6c45f8d6f6
 namespace: sys-mon
 ownerReferences:
 - apiVersion: apps/v1
   blockOwnerDeletion: true
   controller: true
   kind: Deployment
   name: opentelemetry-collector
   uid: f875e269-0e76-4735-a02d-a254ed056111

fatsheep9146 · 2022-09-27T05:29:50Z

I think maybe instead of going this route, we should discover ReplicaSets and get rid of Deployment regex?

The idea behind this, is that regex can match things that are not managed by Deployment, and we would still set Deployment name incorrectly.

So I think we should instead discover ReplicaSets and parse the Pod's Owner References to find the ReplicaSet. Example Pod Owner references:
apiVersion: v1
kind: Pod
metadata:
  labels:
    app.kubernetes.io/instance: opentelemetry-collector
    app.kubernetes.io/name: opentelemetry-collector
    component: standalone-collector
    pod-template-hash: 6c45f8d6f6
  name: opentelemetry-collector-6c45f8d6f6-s4jq4
  ownerReferences:                                                  
  - apiVersion: apps/v1
    blockOwnerDeletion: true
    controller: true 
    kind: ReplicaSet
    name: opentelemetry-collector-6c45f8d6f6
    uid: ee75293d-14ec-42a0-9548-a768d9e07c48
And from ReplicaSet's Owner Reference we can get Deployment name, and id:
apiVersion: apps/v1
kind: ReplicaSet
metadata:
 labels:
   app.kubernetes.io/instance: opentelemetry-collector
   app.kubernetes.io/name: opentelemetry-collector
   component: standalone-collector
   pod-template-hash: 6c45f8d6f6
 name: opentelemetry-collector-6c45f8d6f6
 namespace: sys-mon
 ownerReferences:
 - apiVersion: apps/v1
   blockOwnerDeletion: true
   controller: true
   kind: Deployment
   name: opentelemetry-collector
   uid: f875e269-0e76-4735-a02d-a254ed056111

I think this make senses, and it is the most accurate way to implement this, I'll change like this.
@povilasv @dmitryax

github-actions · 2022-10-11T05:39:54Z

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

fatsheep9146 · 2022-10-11T05:57:18Z

I will start fixing this

fatsheep9146 · 2022-10-12T06:00:03Z

@povilasv I already changed to replicaset, could you help review about this again?

evan-bradley

Looks mostly good to me, just a few comments. Nice work with tracking the relationship between Pods and Deployments using ReplicaSets, I think that's definitely an improvement.

processor/k8sattributesprocessor/internal/kube/client_test.go

processor/k8sattributesprocessor/internal/kube/client.go

processor/k8sattributesprocessor/internal/kube/kube.go

fatsheep9146 · 2022-10-17T05:48:27Z

could you help review this again? @evan-bradley

evan-bradley

Looks mostly good to me, just a few nitpicks mainly around spelling.

processor/k8sattributesprocessor/internal/kube/client.go

.chloggen/k8sattributeprocessor-support-deployment-uid.yaml

unreleased/k8sattributeprocessor-support-deployment-uid.yaml

.chloggen/k8sattributeprocessor-support-deployment-uid.yaml

processor/k8sattributesprocessor/client_test.go

fatsheep9146 · 2022-10-18T23:48:48Z

could you help review this again? @evan-bradley

processor/k8sattributesprocessor/internal/kube/client.go

fatsheep9146 · 2022-10-19T22:54:18Z

could you help review this again? @evan-bradley @povilasv @dmitryax

povilasv

Other than that LGTM, but I am not a maintainer :)

processor/k8sattributesprocessor/internal/kube/client.go

fatsheep9146 · 2022-10-20T09:40:24Z

Other than that LGTM, but I am not a maintainer :)

Any reviews are all welcome, thank you. =D

processor/k8sattributesprocessor/internal/kube/client.go

fatsheep9146 · 2022-10-24T11:10:22Z

Could you help review this again? @evan-bradley

evan-bradley

Looks good to me. Great find with the controller flag in the owner references.

fatsheep9146 · 2022-10-25T01:24:53Z

Do you have time to review this pr ? @dmitryax

dmitryax · 2022-10-25T03:40:51Z

Sorry for the delay. Given that we got some significant changes to this processor that introduces bugs, I'm a bit skeptical about rushing to merge this one.

We discussed with @povilasv plans about introducing e2e tests for k8s related functionality of the collector. Can we delay this PR until the tests are introduced? Let me know if you want to help with that.

dmitryax · 2022-10-25T04:31:05Z

@fatsheep9146 I filed an issue to start with #15651

github-actions · 2022-11-10T05:20:08Z

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

swiatekm · 2023-04-11T13:53:37Z

@dmitryax @fatsheep9146 looks like we now have E2E tests for the k8sattributes processor, so can we revisit this PR?

Having skimmed the code, I have two concerns about the current change:

the Replicaset informer is enabled unconditionally, so users who don't need it will pay the cost regardless
the WatchClient is growing a bit large - we might want to consider moving the responsibility of managing the owner data into a separate component

I don't think these are obstacles to merging this though.

fatsheep9146 · 2023-04-11T14:22:38Z

@dmitryax @fatsheep9146 looks like we now have E2E tests for the k8sattributes processor, so can we revisit this PR?

Having skimmed the code, I have two concerns about the current change:

the Replicaset informer is enabled unconditionally, so users who don't need it will pay the cost regardless

the WatchClient is growing a bit large - we might want to consider moving the responsibility of managing the owner data into a separate component

I don't think these are obstacles to merging this though.

Yeah, you are right, I'will push this pr to be merged in these few days. @swiatekm-sumo

dmitryax · 2023-04-11T23:46:38Z

the Replicaset informer is enabled unconditionally, so users who don't need it will pay the cost regardless

I'd say at least this one should be addressed in this PR. We should avoid putting extra pressure on k8s API and compute resources for the functionality that isn't being used

runforesight · 2023-04-12T05:41:05Z

Foresight Summary

Major Impacts

build-and-test-windows duration(5 seconds) has decreased 30 minutes 41 seconds compared to main branch avg(30 minutes 46 seconds).

View More Details

⭕ build-and-test-windows workflow has finished in 5 seconds (30 minutes 41 seconds less than `main` branch avg.) and finished at 12th Apr, 2023.

Job	Failed Steps	Tests
windows-unittest-matrix	- 🔗	N/A	See Details
windows-unittest	- 🔗	N/A	See Details

✅ check-links workflow has finished in 42 seconds and finished at 12th Apr, 2023.

Job	Failed Steps	Tests
changed files	- 🔗	N/A	See Details
check-links	- 🔗	N/A	See Details

✅ telemetrygen workflow has finished in 1 minute 2 seconds and finished at 12th Apr, 2023.

Job	Failed Steps	Tests
build-dev	- 🔗	N/A	See Details
publish-latest	- 🔗	N/A	See Details
publish-stable	- 🔗	N/A	See Details

✅ prometheus-compliance-tests workflow has finished in 3 minutes 32 seconds (2 minutes 54 seconds less than `main` branch avg.) and finished at 12th Apr, 2023.

Job	Failed Steps	Tests
prometheus-compliance-tests	- 🔗	N/A	See Details

✅ load-tests workflow has finished in 6 minutes 49 seconds (3 minutes 40 seconds less than `main` branch avg.) and finished at 12th Apr, 2023.

Job	Failed Steps	Tests
setup-environment	- 🔗	N/A	See Details
loadtest (TestIdleMode)	- 🔗	N/A	See Details
loadtest (TestBallastMemory\|TestLog10kDPS)	- 🔗	N/A	See Details
loadtest (TestMetric10kDPS\|TestMetricsFromFile)	- 🔗	N/A	See Details
loadtest (TestMetricResourceProcessor\|TestTrace10kSPS)	- 🔗	N/A	See Details
loadtest (TestTraceNoBackend10kSPS\|TestTrace1kSPSWithAttrs)	- 🔗	N/A	See Details
loadtest (TestTraceBallast1kSPSWithAttrs\|TestTraceBallast1kSPSAddAttrs)	- 🔗	N/A	See Details
loadtest (TestTraceAttributesProcessor)	- 🔗	N/A	See Details

✅ changelog workflow has finished in 7 minutes 36 seconds (⚠️ 5 minutes 20 seconds more than `main` branch avg.) and finished at 12th Apr, 2023.

Job	Failed Steps	Tests
changelog	- 🔗	N/A	See Details

❌ e2e-tests workflow has finished in 10 minutes 38 seconds (3 minutes 27 seconds less than `main` branch avg.) and finished at 12th Apr, 2023. 1 job failed.

Job	Failed Steps	Tests
kubernetes-test (v1.26.0)	run e2e tests 🔗	N/A	See Details
kubernetes-test (v1.25.3)	- 🔗	N/A	See Details
kubernetes-test (v1.24.7)	- 🔗	N/A	See Details
kubernetes-test (v1.23.13)	- 🔗	N/A	See Details

❌ build-and-test workflow has finished in 36 minutes 20 seconds (10 minutes 28 seconds less than `main` branch avg.) and finished at 12th Apr, 2023. 2 jobs failed.

Job	Failed Steps	Tests
setup-environment	- 🔗	N/A	See Details
govulncheck	- 🔗	N/A	See Details
check-collector-module-version	Check Collector Module Version 🔗	N/A	See Details
check-codeowners	- 🔗	N/A	See Details
lint-matrix (receiver-0)	- 🔗	N/A	See Details
lint-matrix (receiver-1)	- 🔗	N/A	See Details
lint-matrix (processor)	- 🔗	N/A	See Details
lint-matrix (exporter)	- 🔗	N/A	See Details
lint-matrix (extension)	- 🔗	N/A	See Details
lint-matrix (connector)	- 🔗	N/A	See Details
lint-matrix (internal)	- 🔗	N/A	See Details
lint-matrix (other)	- 🔗	N/A	See Details
checks	Check gendependabot 🔗	N/A	See Details
unittest-matrix (1.20, receiver-0)	- 🔗	N/A	See Details
unittest-matrix (1.20, receiver-1)	- 🔗	N/A	See Details
unittest-matrix (1.20, processor)	- 🔗	N/A	See Details
unittest-matrix (1.20, exporter)	- 🔗	N/A	See Details
unittest-matrix (1.20, extension)	- 🔗	N/A	See Details
unittest-matrix (1.20, connector)	- 🔗	N/A	See Details
unittest-matrix (1.20, internal)	- 🔗	N/A	See Details
unittest-matrix (1.20, other)	- 🔗	N/A	See Details
unittest-matrix (1.19, receiver-0)	- 🔗	N/A	See Details
unittest-matrix (1.19, receiver-1)	- 🔗	N/A	See Details
unittest-matrix (1.19, processor)	- 🔗	N/A	See Details
unittest-matrix (1.19, exporter)	- 🔗	N/A	See Details
unittest-matrix (1.19, extension)	- 🔗	N/A	See Details
unittest-matrix (1.19, connector)	- 🔗	N/A	See Details
unittest-matrix (1.19, internal)	- 🔗	N/A	See Details
unittest-matrix (1.19, other)	- 🔗	N/A	See Details
correctness-metrics	- 🔗	N/A	See Details
build-examples	- 🔗	N/A	See Details
integration-tests	- 🔗	N/A	See Details
correctness-traces	- 🔗	N/A	See Details
unittest (1.20)	- 🔗	N/A	See Details
unittest (1.19)	- 🔗	N/A	See Details
lint	- 🔗	N/A	See Details
cross-compile (darwin, amd64)	- 🔗	N/A	See Details
cross-compile (darwin, arm64)	- 🔗	N/A	See Details
cross-compile (linux, 386)	- 🔗	N/A	See Details
cross-compile (linux, amd64)	- 🔗	N/A	See Details
cross-compile (linux, arm)	- 🔗	N/A	See Details
cross-compile (linux, arm64)	- 🔗	N/A	See Details
cross-compile (linux, ppc64le)	- 🔗	N/A	See Details
cross-compile (windows, 386)	- 🔗	N/A	See Details
cross-compile (windows, amd64)	- 🔗	N/A	See Details
build-package (deb)	- 🔗	N/A	See Details
build-package (rpm)	- 🔗	N/A	See Details
windows-msi	- 🔗	N/A	See Details
publish-check	- 🔗	N/A	See Details
publish-dev	- 🔗	N/A	See Details
publish-stable	- 🔗	N/A	See Details
rotate-milestone	- 🔗	N/A	See Details

🔎 See details on Foresight

^{*You can configure Foresight comments in your organization settings page.}

fatsheep9146 · 2023-04-12T05:44:07Z

the Replicaset informer is enabled unconditionally, so users who don't need it will pay the cost regardless

I'd say at least this one should be addressed in this PR. We should avoid putting extra pressure on k8s API and compute resources for the functionality that isn't being used

I already solved this one.

About another improvement, I think I can submit another pr, since this one already has many modifications.

@dmitryax @swiatekm-sumo

swiatekm

LGTM

fatsheep9146 · 2023-04-17T09:12:16Z

@swiatekm-sumo Thx! The failed test are still needed to be resolved, I'm already doing this.

fatsheep9146 · 2023-04-20T03:34:25Z

@dmitryax The pr is ready to be reviewed.
The failed test is not related to this pr.

processor/k8sattributesprocessor/internal/kube/client.go

dmitryax

Just one nit, otherwise LGTM

processor/k8sattributesprocessor/internal/kube/kube.go

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from 5955b90 to fbe2f33 Compare September 11, 2022 04:11

fatsheep9146 marked this pull request as ready for review September 11, 2022 04:13

fatsheep9146 requested a review from a team September 11, 2022 04:13

fatsheep9146 requested a review from dmitryax as a code owner September 11, 2022 04:13

github-actions bot assigned codeboten Sep 11, 2022

djaglowski added the processor/k8sattributes k8s Attributes processor label Sep 19, 2022

povilasv reviewed Sep 26, 2022

View reviewed changes

github-actions bot added the Stale label Oct 11, 2022

github-actions bot removed the Stale label Oct 12, 2022

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from fbe2f33 to 83a3e0d Compare October 12, 2022 05:59

evan-bradley reviewed Oct 13, 2022

View reviewed changes

evan-bradley reviewed Oct 18, 2022

View reviewed changes

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from e11da04 to bf5afeb Compare October 18, 2022 22:56

povilasv reviewed Oct 19, 2022

View reviewed changes

processor/k8sattributesprocessor/internal/kube/client.go Outdated Show resolved Hide resolved

povilasv approved these changes Oct 20, 2022

View reviewed changes

processor/k8sattributesprocessor/internal/kube/client.go Outdated Show resolved Hide resolved

evan-bradley reviewed Oct 20, 2022

View reviewed changes

processor/k8sattributesprocessor/internal/kube/client.go Outdated Show resolved Hide resolved

processor/k8sattributesprocessor/internal/kube/client.go Outdated Show resolved Hide resolved

processor/k8sattributesprocessor/internal/kube/client.go Outdated Show resolved Hide resolved

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from 44e5ab9 to 88308e1 Compare October 24, 2022 11:08

evan-bradley approved these changes Oct 24, 2022

View reviewed changes

github-actions bot added the Stale label Nov 10, 2022

fatsheep9146 added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Nov 10, 2022

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from e817a19 to 07504e7 Compare April 12, 2023 05:38

github-actions bot requested a review from rmfitzpatrick April 12, 2023 05:39

swiatekm approved these changes Apr 17, 2023

View reviewed changes

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from d64f9d7 to edd9da1 Compare April 23, 2023 04:45

povilasv reviewed Apr 27, 2023

View reviewed changes

processor/k8sattributesprocessor/internal/kube/client.go Outdated Show resolved Hide resolved

processor/k8sattributesprocessor/internal/kube/client.go Outdated Show resolved Hide resolved

processor/k8sattributesprocessor/internal/kube/client.go Outdated Show resolved Hide resolved

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from 848b710 to b915c18 Compare May 15, 2023 14:21

fatsheep9146 requested a review from povilasv May 15, 2023 14:22

dmitryax approved these changes May 16, 2023

View reviewed changes

processor/k8sattributesprocessor/internal/kube/kube.go Outdated Show resolved Hide resolved

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from b915c18 to 8fd3dfb Compare May 16, 2023 01:47

[processor/k8sattribute] support attribute k8s.deployment.uid

a6539c6

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 force-pushed the k8sattributeprocessor-support-deployment-uid branch from 2bbd9b5 to a6539c6 Compare May 16, 2023 04:49

dmitryax approved these changes May 16, 2023

View reviewed changes

dmitryax merged commit 470ae6a into open-telemetry:main May 16, 2023

github-actions bot added this to the next release milestone May 16, 2023

sirianni mentioned this pull request Jun 5, 2023

[processor/k8sattributes] Excessive heap usage from k8sattributes processor after 0.78.0 upgrade #23067

Closed

kamalmarhubi mentioned this pull request Aug 16, 2023

[processor/k8sattributes] Allow adding custom attributes from the ownerReferences chain #25837

Closed

[processor/k8sattribute] support attribute k8s.deployment.uid #14003

[processor/k8sattribute] support attribute k8s.deployment.uid #14003

Conversation

fatsheep9146 commented Sep 9, 2022

fatsheep9146 commented Sep 14, 2022

fatsheep9146 commented Sep 26, 2022

povilasv left a comment • edited Loading

Choose a reason for hiding this comment

fatsheep9146 commented Sep 27, 2022

github-actions bot commented Oct 11, 2022

fatsheep9146 commented Oct 11, 2022

fatsheep9146 commented Oct 12, 2022

evan-bradley left a comment

Choose a reason for hiding this comment

fatsheep9146 commented Oct 17, 2022

evan-bradley left a comment

Choose a reason for hiding this comment

fatsheep9146 commented Oct 18, 2022

fatsheep9146 commented Oct 19, 2022 • edited Loading

povilasv left a comment

Choose a reason for hiding this comment

fatsheep9146 commented Oct 20, 2022

fatsheep9146 commented Oct 24, 2022

evan-bradley left a comment

Choose a reason for hiding this comment

fatsheep9146 commented Oct 25, 2022 • edited Loading

dmitryax commented Oct 25, 2022

dmitryax commented Oct 25, 2022 • edited Loading

github-actions bot commented Nov 10, 2022

swiatekm commented Apr 11, 2023

fatsheep9146 commented Apr 11, 2023

dmitryax commented Apr 11, 2023

runforesight bot commented Apr 12, 2023 • edited Loading

Foresight Summary

build-and-test-windows duration(5 seconds) has decreased 30 minutes 41 seconds compared to main branch avg(30 minutes 46 seconds).

⭕ build-and-test-windows workflow has finished in 5 seconds (30 minutes 41 seconds less than main branch avg.) and finished at 12th Apr, 2023.

✅ check-links workflow has finished in 42 seconds and finished at 12th Apr, 2023.

✅ telemetrygen workflow has finished in 1 minute 2 seconds and finished at 12th Apr, 2023.

✅ prometheus-compliance-tests workflow has finished in 3 minutes 32 seconds (2 minutes 54 seconds less than main branch avg.) and finished at 12th Apr, 2023.

✅ load-tests workflow has finished in 6 minutes 49 seconds (3 minutes 40 seconds less than main branch avg.) and finished at 12th Apr, 2023.

✅ changelog workflow has finished in 7 minutes 36 seconds (⚠️ 5 minutes 20 seconds more than main branch avg.) and finished at 12th Apr, 2023.

❌ e2e-tests workflow has finished in 10 minutes 38 seconds (3 minutes 27 seconds less than main branch avg.) and finished at 12th Apr, 2023. 1 job failed.

❌ build-and-test workflow has finished in 36 minutes 20 seconds (10 minutes 28 seconds less than main branch avg.) and finished at 12th Apr, 2023. 2 jobs failed.

fatsheep9146 commented Apr 12, 2023 • edited Loading

swiatekm left a comment

Choose a reason for hiding this comment

fatsheep9146 commented Apr 17, 2023

fatsheep9146 commented Apr 20, 2023

dmitryax left a comment

Choose a reason for hiding this comment

povilasv left a comment •

edited

Loading

fatsheep9146 commented Oct 19, 2022 •

edited

Loading

fatsheep9146 commented Oct 25, 2022 •

edited

Loading

dmitryax commented Oct 25, 2022 •

edited

Loading

runforesight bot commented Apr 12, 2023 •

edited

Loading

`build-and-test-windows` duration(5 seconds) has decreased 30 minutes 41 seconds compared to main branch avg(30 minutes 46 seconds).

⭕ build-and-test-windows workflow has finished in 5 seconds (30 minutes 41 seconds less than `main` branch avg.) and finished at 12th Apr, 2023.

✅ prometheus-compliance-tests workflow has finished in 3 minutes 32 seconds (2 minutes 54 seconds less than `main` branch avg.) and finished at 12th Apr, 2023.

✅ load-tests workflow has finished in 6 minutes 49 seconds (3 minutes 40 seconds less than `main` branch avg.) and finished at 12th Apr, 2023.

✅ changelog workflow has finished in 7 minutes 36 seconds (⚠️ 5 minutes 20 seconds more than `main` branch avg.) and finished at 12th Apr, 2023.

❌ e2e-tests workflow has finished in 10 minutes 38 seconds (3 minutes 27 seconds less than `main` branch avg.) and finished at 12th Apr, 2023. 1 job failed.

❌ build-and-test workflow has finished in 36 minutes 20 seconds (10 minutes 28 seconds less than `main` branch avg.) and finished at 12th Apr, 2023. 2 jobs failed.

fatsheep9146 commented Apr 12, 2023 •

edited

Loading