Auto-inject the IP tag for operator-injected agent #871
Conversation
pkg/inject/sidecar.go
Outdated
| Name: envVarPodIp, | ||
| ValueFrom: &corev1.EnvVarSource{ | ||
| FieldRef: &corev1.ObjectFieldSelector{ | ||
| FieldPath: "status.podIP", |
There was a problem hiding this comment.
I think the original request was to have the host IP, not the pod. IIRC, the pod IP will already be reported as part of the process tags.
cc @TBBle
There was a problem hiding this comment.
Haaa you are totally right! Changed.
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
rubenvp8510
force-pushed
the
Issue-865
branch
from
d8efaa9 to
99a0d66
Compare
rubenvp8510
force-pushed
the
Issue-865
branch
from
99a0d66 to
61cfa50
Compare
pkg/inject/sidecar.go
Outdated
| "cluster", "undefined", // this value isn't currently available | ||
| "deployment.name", dep.Name, | ||
| "pod.namespace", dep.Namespace, | ||
| "pod.name", fmt.Sprintf("${%s:}", envVarPodName), | ||
| "ip", fmt.Sprintf("${%s:}", envVarHostIP), |
There was a problem hiding this comment.
So it's not lost in the other thread, this needs to go back to host.ip, as when a tracer library submits traces with a process tag ip, the resulting trace will end up with two such tags, rather than one suppressing the other as I had assumed. i.e. 99a0d66 was fine.
rubenvp8510
force-pushed
the
Issue-865
branch
from
61cfa50 to
99a0d66
Compare
|
After read all the discussion I reverted the last change , back to |
There was a problem hiding this comment.
LGTM. I'm OK with this change, but would be nice to also get feedback from @pavolloffay or @objectiser about whether or not this is appropriate. My concern is that we might be getting close to the point where we add too much stuff to the span that we might not need.
There was a problem hiding this comment.
@jpkrohling @rubenvp8510 Another option (for now) would be to keep the environment variable, but not implicitly add it to the list of preconfigured jaeger-tags. The user could then define their own jaeger tags list, using the tag name they want.
|
I've be fine with having the env-var and manually using it, but it'd be nice if this was either the case for all the parameters, or the agent args in the My preference is to keep 'em all, or trim ones that aren't valuable though. Less configuration (and hence less teaching of configuration) makes me a happier developer. ^_^ |
|
@TBBle Agree it might be nice to add new |
|
I'd be OK with merging this PR as is, and see if we have a need for further tags in the future. This might be the last one we can automatically add anyway... |
|
The other thing to keep an eye on is OpenTelemetry: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-resource-semantic-conventions.md IP attribute is not covered currently, but |
|
That document suggests to me that we should be (later...) exposing all these attributes as env-vars, and then That said, I understand OpenTelemetry has plans for their own agents, so it might never be a super-interesting use-case for this part of the Jaeger Operator, but remains academically interesting. |
|
@TBBle, @objectiser : what's the verdict then? |
|
I think the agreement was to merge as-is, unless you have changed your mind? |
|
No, just wanted to confirm before merging :-) |
Fixes #865
Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com