Choose target container injection with annotation #689

fscellos · 2022-02-04T14:57:36Z

This PR address the same problem of the following PR (#683) without introducing big changes in codebase.
I just introduce a new annotation near existing auto-inject instrumenation that allow to search first for a corresponding containername before fallback to the first one by default :

instrumentation.opentelemetry.io/container-name: autodeploy
instrumentation.opentelemetry.io/inject-java: "true"

This approach present the advantage to let end user to define exact injection target (as is some cases, cluster administrator won't want user to manipulate Instrumentation ressources to keep control on sampling rate)

Resolves #618

linux-foundation-easycla · 2022-02-04T14:57:40Z

The committers are authorized under a signed CLA.

✅ fscellos (db7ecc3)

VineethReddy02 · 2022-02-05T20:30:23Z

@pavolloffay can you review these changes as they are around auto-instrumentation?

pavolloffay · 2022-02-07T13:48:55Z

@fscellos does this PR implement the discussed approach from
#529 (comment) ?

fscellos · 2022-02-07T13:58:38Z

Hello. Not really.
The simple case i wan't to adress is the common case of a pod that contains multiple containers (in particular istio-proxy that always get the 0-position) and i want to precise to instrumentation on which container to inject env variables.
I try it on a real application with istio enabled with success but i notice i have some other properties to update too (the container tag name for span is always set to the first container, not the one i choose)

pavolloffay · 2022-02-11T15:25:17Z

This PR is missing e2e test, this is a major feature that deserves it.

pkg/instrumentation/sdk.go

pavolloffay · 2022-02-11T16:29:12Z

@anuraaga @BronzeDeer could you please take a look at the proposal in this PR?

The proposal is to basically add an optional annotation e.g. instrumentation.opentelemetry.io/container-name: app to specify the container for injection. If annotation is not present injection to the first container is applied.

The original proposal from #529 (comment) allows specifying container name per language.

BronzeDeer · 2022-02-12T09:19:18Z

This PR solves the problem for the special case of injecting into a single container that is not the first, while the suggestion from #529 and PR #683 is the more general solution which allows to configure injection of arbitrary languages into arbitrary many containers at the cost of a slightly increased complexity (allthough not much IMO, its basically filter and map over the container list with the existing injection function, you could even argue that it serves to decouple the injection logic from pod modification logic)

Which solution is the correct one in the long-term probably comes down more to whether the idea of injecting multiple containers with potentially different languages in a single pods is a probable use case or falls under YAGNI. Making that call likely falls solely on the shoulders of the maintainers/stewards of the project

fscellos · 2022-02-12T10:18:12Z

That's ok @BronzeDeer, i just adress problemematic of injection with istio sidecar.
I do that because i don't think that a lot of user use multi-container pods with multiple technology.
Common use case is a sidecar /ambassador pattern with often containers technology that don't support auto-injection (C/C++ or go).
I'm agree it is not so difficult to do but we have to consider balance between configuration "complexity" (many annotations) and usefullness.
(the only use case i see where it can be useful is for some pods like in eclipse che (pods with many containers); but i don't think this kind of pods is a target for instrumentation (just dev env))

BronzeDeer · 2022-02-16T19:56:52Z

@pavolloffay @anuraaga could you comment on the design decision between the simple and more general case? And if you decide we stick with the more general approach, could you answer my two small questions in #683, so I can get that PR ready?

pavolloffay · 2022-02-22T08:59:39Z

I am fine to move this forward. Right now we don't have any requests to support instrumenting a pod with 2 different languages (e.g. java and nodejs containers). If this requirements comes we could introduce more specific container name annotation per language:

instrumentation.opentelemetry.io/inject-java-container-names = "spring-petclinic"

README.md

pavolloffay · 2022-03-09T15:16:09Z

@fscellos any update on this PR?

fscellos · 2022-03-09T15:19:25Z

Hello @pavolloffay, not now.
I keep it in mind but have to deal with one other priority before. I'll keep you inform.

fscellos · 2022-03-15T12:40:57Z

Hello @pavolloffay : i can begin to work again on it again. I'll keep you inform.

fscellos · 2022-03-16T17:05:14Z

Hello @pavolloffay : i just updated PR with :

Merge main branch of main repo
Taking account of your first remarks about chooseServiceName and createResourceMap
Adding ability to declare multiple container names for injection (of same type).
Adding some integration tests

README.md

pavolloffay · 2022-04-01T08:10:01Z

README.md

+```
+In case of injection on multiple containers, you can indicates all your containers'name separate with comma.
+
+#### Use customized or vendor instrumentation


This change should not be here, maybe the PR was not rebased properly?

This section should not be added in this PR

OK. I didn't understand you don't want any modifications in documentation.
I remove all of this and get Readme from main branch of main repo.

tests/e2e/instrumentation-nodejs-multicontainer/NAR.YAML

pkg/instrumentation/sdk.go

pavolloffay · 2022-04-01T08:28:50Z

@fscellos thanks for working on this, I have left some comments. I would like to move this PR forward, let me know if you have time or I should take over.

fscellos · 2022-04-01T09:34:33Z

Hello @pavolloffay.
I will take a closer look on it next monday.

fscellos · 2022-04-04T08:42:44Z

Hello @pavolloffay.
I push asked modification and let you check if all is ok for you.

jpkrohling · 2022-04-05T19:30:23Z

Tests started.

fscellos · 2022-04-06T07:30:04Z

Hello @pavolloffay , @jpkrohling. I correct the two linting problems.
Sorry i miss it on local before my commit as i didn't know i have to launch it explicitly

pavolloffay · 2022-04-07T09:51:01Z

config/manager/kustomization.yaml

@@ -1,2 +1,8 @@
 resources:
-  - manager.yaml
+- manager.yaml


this file shouldn't be changed in this PR

Yes. I don't understand what happens locally. I keep file from main branch of principal repository.
Apologize.

It don't know why but it seems that something in prepare-e2e (or e2e) targets, something modify this file.
I didn't commit it again.

pavolloffay · 2022-04-07T09:51:32Z

README.md

+```
+In case of injection on multiple containers, you can indicates all your containers'name separate with comma.
+
+#### Use customized or vendor instrumentation


This section should not be added in this PR

pavolloffay · 2022-04-07T09:53:02Z

@fscellos please clean the PR - readme and kustomization.yaml and also the lint failed. Otherwise it looks good and we an merge it.

fscellos · 2022-04-07T16:40:26Z

Hello @pavolloffay.
I commit the two files.
For linter i launch it locally through docker with all linter activated (docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.45.2 golangci-lint run -v --timeout 20m / timeout is huge because i had connectivity problem today) and i don't find any problem (in CI errors seems to be about a class i didn't modify; maybe a temporary problem in CI build ?)

pavolloffay

PR needs to be rebased and we should add the annotation to the root readme.

pavolloffay · 2022-04-22T07:27:46Z

README.md

@@ -258,6 +258,8 @@ The OpenTelemetry Operator *might* work on versions outside of the given range,

 | OpenTelemetry Operator | Kubernetes           | Cert-Manager         |
 |------------------------|----------------------|----------------------|
+| v0.48.0                | v1.19 to v1.23       | 1.6.1                |


There should not be changes in this support table.

I rebased. I will let you add documentation on your convenience (I already added it then remove)

pavolloffay · 2022-04-25T10:47:20Z

@fscellos now the CI is failing :/

fscellos · 2022-04-25T11:54:59Z

Hello @pavolloffay . Yes i saw that. I would check it but impossible to pull any image from github repo this morning (so all e2e failed on my side too).
I will recheck it in a few hours.

fscellos · 2022-04-25T12:24:39Z

Hello @pavolloffay .
No chance, but i confirm you that we probably encountered a problem with github registry for previous build.
I rerun to launch e2e test and all seems to be ok now without any modifications on code; it is possible for you to relaunch e2e tests on CI ?

pavolloffay · 2022-04-29T06:22:22Z

@fscellos I have restarted failed jobs.

However the PR needs to be rebased again :/.

Choose container injection by annotation

fscellos · 2022-04-29T08:40:42Z

Hello @pavolloffay. Rebase done.
I launch e2e tests on kind cluster 1.19 & 1.23 locally before.
I hope it is last rebase/commit :-).. On May i will have conferences to see in valence (i see you'll have a session there) and get less time ;-)

fscellos · 2022-05-10T08:02:19Z

Hello @pavolloffay : any news on review ?

pavolloffay

@fscellos great work!

I think we have lost the readme documentation for this feature during the rebasing. I will merge it without the docs, but please open a follow up PR to document this.

fscellos · 2022-05-13T14:58:26Z

Ok. I think i didin't understand one of your remarks because i explicitlty remove documention about this feature.
I will open another PR for documentation monday 16.

Choose container injection by annotation

fscellos requested review from a team and bogdandrutu February 4, 2022 14:57

fscellos changed the title ~~Integration du choix du conteneur par annotation~~ Choose target container injection with annotation Feb 4, 2022

VineethReddy02 requested a review from pavolloffay February 5, 2022 20:30

pavolloffay reviewed Feb 11, 2022

View reviewed changes

pkg/instrumentation/sdk.go Show resolved Hide resolved

pavolloffay reviewed Feb 22, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

BronzeDeer mentioned this pull request Feb 25, 2022

[WIP] auto-instrumentation: Allow per container control #683

Closed

pavolloffay closed this Apr 1, 2022

pavolloffay reopened this Apr 1, 2022

pavolloffay reviewed Apr 1, 2022

View reviewed changes

pavolloffay reviewed Apr 7, 2022

View reviewed changes

frzifus mentioned this pull request Apr 11, 2022

Optional auto-update on instrumentation change #817

Closed

2 tasks

pavolloffay reviewed Apr 22, 2022

View reviewed changes

fscellos force-pushed the feature-instrumentation-annotation-select-container-name branch from 09721f5 to e84c061 Compare April 25, 2022 07:43

parent 3c631a3

1291dcd

Choose container injection by annotation

fscellos force-pushed the feature-instrumentation-annotation-select-container-name branch from e84c061 to 1291dcd Compare April 29, 2022 08:20

pavolloffay approved these changes May 13, 2022

View reviewed changes

pavolloffay merged commit b4bd108 into open-telemetry:main May 13, 2022

pavolloffay mentioned this pull request May 13, 2022

OTEL Operator release 0.51.0 #869

Merged

pavolloffay mentioned this pull request May 13, 2022

e2e tests are failing #871

Closed

fscellos mentioned this pull request May 16, 2022

Documentation for container-names annotation #876

Merged

jasaltvik mentioned this pull request May 19, 2022

Update Community OpenTelemetry Operator on OperatorHub to v0.51.0 #883

Closed

ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024

Choose target container injection with annotation (open-telemetry#689)

66a67d3

Choose container injection by annotation

Choose target container injection with annotation #689

Choose target container injection with annotation #689

Conversation

fscellos commented Feb 4, 2022 • edited by pavolloffay Loading

linux-foundation-easycla bot commented Feb 4, 2022 • edited Loading

VineethReddy02 commented Feb 5, 2022

pavolloffay commented Feb 7, 2022

fscellos commented Feb 7, 2022 • edited Loading

pavolloffay commented Feb 11, 2022

pavolloffay commented Feb 11, 2022

BronzeDeer commented Feb 12, 2022

fscellos commented Feb 12, 2022 • edited Loading

BronzeDeer commented Feb 16, 2022

pavolloffay commented Feb 22, 2022

pavolloffay commented Mar 9, 2022

fscellos commented Mar 9, 2022 • edited Loading

fscellos commented Mar 15, 2022

fscellos commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Apr 1, 2022

fscellos commented Apr 1, 2022

fscellos commented Apr 4, 2022

jpkrohling commented Apr 5, 2022

fscellos commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Apr 7, 2022

fscellos commented Apr 7, 2022

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Apr 25, 2022

fscellos commented Apr 25, 2022

fscellos commented Apr 25, 2022

pavolloffay commented Apr 29, 2022

fscellos commented Apr 29, 2022

fscellos commented May 10, 2022

pavolloffay left a comment

Choose a reason for hiding this comment

fscellos commented May 13, 2022

fscellos commented Feb 4, 2022 •

edited by pavolloffay

Loading

linux-foundation-easycla bot commented Feb 4, 2022 •

edited

Loading

fscellos commented Feb 7, 2022 •

edited

Loading

fscellos commented Feb 12, 2022 •

edited

Loading

fscellos commented Mar 9, 2022 •

edited

Loading