-
Notifications
You must be signed in to change notification settings - Fork 414
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
using opentelemetry.io* namespace for extracing resource and attribute labels from k8s Api #2330
Conversation
pkg/instrumentation/sdk.go
Outdated
return true, err | ||
} | ||
if err != nil { | ||
return false, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this line is not reachable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Horiodino looks like some import failures. Also, please sign the CLA when you get a chance :)
… and attribute labels from Kubernetes API open-telemetry#2330
pkg/instrumentation/sdk.go
Outdated
@@ -402,6 +402,54 @@ func chooseServiceVersion(pod corev1.Pod, index int) string { | |||
return tag | |||
} | |||
|
|||
// create/reserve namespace | |||
|
|||
func (i *sdkInjector) createReservedNamespace(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to create a namespace here?
pkg/instrumentation/sdk.go
Outdated
} | ||
|
||
// recognizeUserDefinedValues recognizes user defined values assigned with opentelemetry.io/ prefix | ||
func (i *sdkInjector) recognizeUserDefinedValues(ctx context.Context, ns corev1.Namespace, pod corev1.Pod, deployment appsv1.Deployment, index int) map[string]string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add some tests in? and make it so that this function is used as part of the injection process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok 👍
fcb61a7
to
886da91
Compare
@jaronoff97 @frzifus i have implemented it for mostly all resources, like pod , deply,statefulset, replicas etc..., but what i have done is i created a one main function that returns all info in map but the thing is in function arguments i need k8s client , should i separate create all functions for all resources ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Horiodino per my last comment (link) there is no need to reach out to the kube API for any of this. Further, you're not using the functions you wrote and they are currently untested. If you'd like, I'd be happy to pair with you on this PR and we can work through it together.
sure ✨ |
feel free to reach out via the CNCF slack and we can set up some time |
@Horiodino would you be able to fix the merge conflicts, fix the unit tests, and add a changelog? I would love to get this merged in, it's looking pretty good! |
@Horiodino bump on this, I'd love to get this in. |
sorry for taking long , tried testing but that failed due to my system (previously using 32), |
hey @jaronoff97 #3010 |
…notations from k8s Api #2330 (#3010) * using opentelemetry.io* namespace for extracing resource Signed-off-by: Horiodino <[email protected]> * refactor ReservedNamespace to OtelAnnotationNamespace Signed-off-by: Horiodino <[email protected]> * added changelog Signed-off-by: Horiodino <[email protected]> * Updated README: using opentelemetry.io* namespace for extracting resource Signed-off-by: Horiodino <[email protected]> * Update README.md Co-authored-by: Mikołaj Świątek <[email protected]> * Update README.md Co-authored-by: Mikołaj Świątek <[email protected]> --------- Signed-off-by: Horiodino <[email protected]> Co-authored-by: Mikołaj Świątek <[email protected]>
Description:
i have added recognizeUserDefinedValues() that will recognize the pods with pentelemetry.io/ prefix
#2181
Testing: i haven't tested it yet , im not sure is it right so please tell any changes to made