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

feat: webhook to set agent injection env var with odigos additions #2107

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

tamirdavid1
Copy link
Collaborator

@tamirdavid1 tamirdavid1 commented Dec 31, 2024

This is the initial phase of enabling the webhook to set agent injection-related environment variables (e.g., PYTHONPATH). The plan is to merge this after the migration and the new runtime inspection have been running for a while.

Once enough time has passed, I will merge this change to handle simpler cases where the envOverwriter is not involved. After this phase runs for a sufficient period, we can fully remove the envOverwriter and rely solely on this logic.

Finally, we can clean up the remaining code related to the envOverwriter.

@@ -52,6 +53,15 @@ type InstrumentationConfigStatus struct {
RuntimeDetailsByContainer []RuntimeDetailsByContainer `json:"runtimeDetailsByContainer,omitempty"`
}

func (in *InstrumentationConfigStatus) GetRuntimeDetailsForContainer(container v1.Container) *RuntimeDetailsByContainer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually host these sort of functions in k8sutils. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we discussed let's discuss on this with @RonFed and take decision here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm OK with both options as long we don't have duplicated functions in more than one place.
It feels like we have a lot of functions like this in

func getLanguageOfContainer(runtimeDetails []odigosv1.RuntimeDetailsByContainer, containerName string) common.ProgrammingLanguage {

func getContainerOtherAgents(runtimeDetails []odigosv1.RuntimeDetailsByContainer, containerName string) *odigosv1.OtherAgent {

func getLibCTypeOfContainer(runtimeDetails []odigosv1.RuntimeDetailsByContainer, containerName string) *common.LibCType {

k8sutils/pkg/container/container.go Outdated Show resolved Hide resolved
@tamirdavid1 tamirdavid1 force-pushed the inject-env-new-runtime-logic branch from 67512da to d5924cc Compare January 1, 2025 15:59
@@ -52,6 +53,15 @@ type InstrumentationConfigStatus struct {
RuntimeDetailsByContainer []RuntimeDetailsByContainer `json:"runtimeDetailsByContainer,omitempty"`
}

func (in *InstrumentationConfigStatus) GetRuntimeDetailsForContainer(container v1.Container) *RuntimeDetailsByContainer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm OK with both options as long we don't have duplicated functions in more than one place.
It feels like we have a lot of functions like this in

func getLanguageOfContainer(runtimeDetails []odigosv1.RuntimeDetailsByContainer, containerName string) common.ProgrammingLanguage {

func getContainerOtherAgents(runtimeDetails []odigosv1.RuntimeDetailsByContainer, containerName string) *odigosv1.OtherAgent {

func getLibCTypeOfContainer(runtimeDetails []odigosv1.RuntimeDetailsByContainer, containerName string) *common.LibCType {

@damemi
Copy link
Contributor

damemi commented Jan 4, 2025

Would you please update the test cases re:reported-name like I did in 8b60b1d ? You can probably just cherry pick that commit

@tamirdavid1 tamirdavid1 force-pushed the inject-env-new-runtime-logic branch from 594b83a to b01a9dc Compare January 5, 2025 07:53
@tamirdavid1
Copy link
Collaborator Author

Would you please update the test cases re:reported-name like I did in 8b60b1d ? You can probably just cherry pick that commit
Yea I can do that, can you elaborate about why 26 spans reduced to 13?

@damemi
Copy link
Contributor

damemi commented Jan 5, 2025

Yea I can do that, can you elaborate about why 26 spans reduced to 13?

The original test used the same resource names for 2 runs of traffic generation. So the first time it would generate 13 spans and then 13 more. But now the test updates the reported-name after the first set is generated, so the new set of spans is just 13 entirely new resources.

@tamirdavid1 tamirdavid1 force-pushed the inject-env-new-runtime-logic branch from 5770291 to f67f0ba Compare January 6, 2025 11:13
@tamirdavid1
Copy link
Collaborator Author

Yea I can do that, can you elaborate about why 26 spans reduced to 13?

The original test used the same resource names for 2 runs of traffic generation. So the first time it would generate 13 spans and then 13 more. But now the test updates the reported-name after the first set is generated, so the new set of spans is just 13 entirely new resources.

Done, the requested commit has been cherry-picked.

@BenElferink BenElferink added the enhancement New feature or request label Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants