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: Collect Language Version in Runtime #1434

Merged
merged 16 commits into from
Aug 15, 2024

Conversation

yodigos
Copy link
Collaborator

@yodigos yodigos commented Aug 8, 2024

No description provided.

@blumamir blumamir changed the title feart: Collect Language Version in Runtime feat: Collect Language Version in Runtime Aug 8, 2024
Copy link
Collaborator

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM
left few minor comments

common/lang_detection.go Outdated Show resolved Hide resolved
common/envs/detailedEnvs.go Outdated Show resolved Hide resolved
common/envs/detailedEnvs.go Outdated Show resolved Hide resolved
procdiscovery/pkg/process/process.go Outdated Show resolved Hide resolved
procdiscovery/pkg/inspectors/golang/golang.go Outdated Show resolved Hide resolved
procdiscovery/pkg/inspectors/java/java.go Show resolved Hide resolved
common/envs/detailedEnvs.go Outdated Show resolved Hide resolved
procdiscovery/pkg/process/process.go Outdated Show resolved Hide resolved
const JavaVersionConst = "JAVA_VERSION"

// envDetailsSeparatorMap is a map of environment variables and their separators
var envDetailsSeparatorMap = []string{NodeVersionConst, PythonVersionConst, JavaVersionConst}
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 this is a slice not a map

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

haha, yeah it was a map and I changed it to a slice, forgot to change the name

}

for _, env := range envDetailsSeparatorMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider converting envDetailsSeparatorMap from slice to map[strings]struct{}
This way you can look directly the keys instead of iterating the map every time

Comment on lines 117 to 119
envs = make([]odigosv1.EnvVar, 0, len(inspectProc.Environments.OverwriteEnvs))
for envName, envValue := range inspectProc.Environments.OverwriteEnvs {

Copy link
Contributor

Choose a reason for hiding this comment

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

This will panic if getRelevantEnvVars returns nil

@edeNFed
Copy link
Contributor

edeNFed commented Aug 11, 2024

I don't think this is a good fix for the nil issue.
Instead of assuming everywhere in the code the engineer will remember to check the Environments is not nil we should make a design that avoids this problem. This can be done by not exporting the Environments variable or allowing access to the nested env vars via functions or changing it from pointer to struct, but as long as instProc.Environments.somethingsomething can be called it is just a matter of time when someone will write a logic that forgets to check for nil and will cause the entire app to crash in case we can't read the environ file

@@ -10,12 +10,34 @@ import (
"github.com/odigos-io/odigos/common/envOverwrite"
)

const NodeVersionConst = "NODE_VERSION"
const PythonVersionConst = "PYTHON_VERSION"
const JavaVersionConst = "JAVA_VERSION"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be defined per language and not here

procdiscovery/pkg/inspectors/dotnet/dotnet.go Show resolved Hide resolved
const PythonVersionConst = "PYTHON_VERSION"
const JavaVersionConst = "JAVA_VERSION"

// envDetailsMap is a map of environment variables and their separators
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// envDetailsMap is a map of environment variables and their separators
// envDetailsMap is a map of environment variables used to discover additional details on the runtime

@yodigos yodigos merged commit 9fe3d90 into odigos-io:main Aug 15, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants