Fix service name source being added multiple times for multiple injections #254
Conversation
pkg/instrumentation/sdk.go
Outdated
| @@ -263,8 +263,22 @@ func (i *sdkInjector) injectCommonSDKConfig(ctx context.Context, otelinst v1alph | |||
| } | |||
| } | |||
|
|
|||
| // get existing resources env var and parse it into a map | |||
| existingRes := map[string]string{} | |||
| existingResourceEnvIdx := getIndexOfEnv(pod.Spec.Containers[appIndex].Env, constants.EnvOTELResourceAttrs) | |||
There was a problem hiding this comment.
seems like this code borrows the first few lines of createResourceMap...can we prevent duplicate code by creating a helper function for the following:
// get existing resources env var and parse it into a map
existingRes := map[string]bool{}
existingResourceEnvIdx := getIndexOfEnv(pod.Spec.Containers[index].Env, constants.EnvOTELResourceAttrs)
if existingResourceEnvIdx > -1 {
existingResArr := strings.Split(pod.Spec.Containers[index].Env[existingResourceEnvIdx].Value, ",")
for _, kv := range existingResArr {
keyValueArr := strings.Split(strings.TrimSpace(kv), "=")
if len(keyValueArr) != 2 {
continue
}
existingRes[keyValueArr[0]] = true
}
}
res := map[string]string{}
for k, v := range otelinst.Spec.Resource.Attributes {
if !existingRes[k] {
res[k] = v
}
}
and then using this in createResourceMap?
There was a problem hiding this comment.
We are using map[string]bool in createResourceMap and in the new code we use map[string]string{}.
There was a problem hiding this comment.
It doesn't need to be a map[string]string{} right? Because we are just checking if it exists.
I don't see us using the value of existingRes at all in the code
There was a problem hiding this comment.
We could actually just modify createResourceMap to return the existing map. Seems wasteful to execute the same code twice.
pkg/instrumentation/sdk.go
Outdated
| // Some attributes might be empty, we should get them via k8s downward API | ||
| if resourceMap[string(semconv.K8SPodNameKey)] == "" { | ||
| if existingRes[string(semconv.K8SPodNameKey)] == "" && resourceMap[string(semconv.K8SPodNameKey)] == "" { |
There was a problem hiding this comment.
Not sure I understand...why isn't resourceMap sufficient to determine if the k8s pod name key is missing?
There was a problem hiding this comment.
for k, v := range otelinst.Spec.Resource.Attributes {
if !existingRes[k] {
res[k] = v
}
}
If the attribute doesn't exist in existingRes, then the res will be updated. So when the second injection happens, res doesn't have the semconv.K8SPodNameKey key in it as it was existing value. So just checking the resourceMap[string(semconv.K8SPodNameKey)] == "" will be true and the key is added again
…er (#262) * added agent server port as default port on CW agent (#234) * Attach service name source as resource attribute to operator (#235) * Add service name source for instrumentation and workload * Change service name source attribute naming * Fix unit tests for pod mutators * Simplify service name source determination * Fix incorrect refactor of source constants * Use replicaset name as part of service name fallback (#249) * Use replicaset name as part of service name fallback * Add chooseServiceName unit test * Change import statement ordering * Fix service name source being added multiple times for multiple injections (#254) * added fix to not override the service name source for multiple auto-inject --------- Co-authored-by: POOJA REDDY NATHALA <poojardy@amazon.com> Co-authored-by: zhihonl <61301537+zhihonl@users.noreply.github.com>
Issue #, if available:
With the current code, if the customer pod has multiple injections , the service name source is being added multiple times and with invalid value from the second injection.
Description of changes:
added code to check the existing OTEL resource attributes and add the attribute only when the existing value is NULL
Tested using
Unit TestsManual Testing on petclinic pod without custom service name
Before
After
With Custom service name:
After
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.