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.
It looks like this line is not reachable
There was a problem hiding this comment.
@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.
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.
could you add some tests in? and make it so that this function is used as part of the injection process
Horiodino
force-pushed
the
main
branch
2 times, most recently
from
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.
@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