Handle case where multiple instrumentations can be applied to a pod

[anuraaga]

Currently, both inject-java and inject-nodejs can be applied to the same pod. This creates some awkwardness, especially if the instrumentation instance referenced is different, common SDK configuration will be applied from two unrelated instrumentations.

One approach that could solve this is to split the current annotations into "whether to inject" and "what language to inject"

auto.opentelemetry.io/inject = "true"
auto.opentelemetry.io/lanuage = "java"

Originally posted by @anuraaga in #507 (comment)

[jpkrohling]

Can we create a validation webhook that will block the creation of the podspec in that case?

[anuraaga]

Validating is definitely an option. Personally I don't see a real advantage to validate / errors vs just structuring it into the annotations though where it stops being a problem. The assumption is two annotations is equivalent to one since they're almost always copy-pasted.

[pavolloffay]

I think allowing multiple injections to a single pod can be actually useful. I have this use-case in mind:

A pod with {java,python,whatnot} application container is instrumented but the pod also has a sidecar proxy (e.g. envoy) that does tracing as well (e.g. to capture payloads or headers). Now user might want to inject javaagent into the first application container and then sdk configuration to the proxy sidecar container. The injection of SDK configuration is used by proxy to configure tracing in the proxy.

[anuraaga]

@pavolloffay Any ideas on structuring the annotations to support multiple containers? Perhaps something like this?

auto.opentelemetry.io/inject = "my-instrumentation"
auto.opentelemetry.io/inject-sdk = "java"

auto.opentelemetry.io/inject = "my-instrumentation"
auto.opentelemetry.io/inject-sdk/app-container = "java"
auto.opentelemetry.io/inject-sdk/proxy-container = "go"

Or do you think it's valid to inject different OTel SDK settings into different containers in the pod? In that case I guess

auto.opentelemetry.io/inject-java = "my-instrumentation"

auto.opentelemetry.io/inject-java/app-container = "my-instrumentation"
auto.opentelemetry.io/inject-go/proxy-container = "my-other-instrumentation"

And if multiple instrumentation names were defined for a single container, we'd need to log an error and disable injection. I guess both of these seem OK.

[anuraaga]

@pavolloffay @BronzeDeer Any thoughts on my syntax proposal?

[pavolloffay]

Or do you think it's valid to inject different OTel SDK settings into different containers in the pod? In that case I guess

It gives more flexibility, but I don't know/doubt it is needed. A use-case might be to use a different sampling rate for init and the main container.

small nit to the annotation name. I think it should be instrumentation.opentelemetry.io/ to match the kind.group semantics.

The most I like is

instrumentation.opentelemetry.io/inject-{java,python,nodejs} = "my-instrumentation" // -injects to first or all? 
instrumentation.opentelemetry.io/inject-{java,python,nodejs}/{container-name} = "my-instrumentation"

cc) @BronzeDeer this is highly related to your proposal in #618

[BronzeDeer]

@anuraaga @pavolloffay Is there a precedent for "double slash" annotations in k8s? The way I read it, this would imply a hierarchical structure between values.
I think this is somewhat at odds with:

1. annotations being "simple" kv pairs, that are optionally "namespaced" by kind.group
2. The example setting both a value for the top-level key, and for sub-keys, meaning you could not even express this datastructure as dict of dicts

I'd replace the second slash with a dash and change it to container-names allowing for a comma delimited list. This would allow specifying exactly which container gets which language instrumentation, with each language instrumentation tied to either a specific Instrumentation CR or the namespace default.

Intuitively I don't see a use case in which it would be necessary to inject two containers of the same language with different sdk versions, if there is a major compatibility breakage in an sdk, I'd prefer to go with something like "java-v2" in a new set of annotations and CRs.

[pavolloffay]

@BronzeDeer I am maybe a bit lost. Coul you please write an example of the annotations in a similar format?

I'd replace the second slash with a dash

Please provide an example how it would look like. In general I like more / as it clearly distinguishes parts of the annotation.

[BronzeDeer]

@pavolloffay Many things in Kubernetes follow a "<namespace>/<name>" pattern, with the "<namespace>/" prefix being optional. Beyond the namespace grouping in my mental model everything else about annotations/labels etc is a flat key-value structure. I'm not sure if we should try to flatten a hierarchical datastructure into the annotation space.

My second objection was about your third suggestion

instrumentation.opentelemetry.io/inject-{java,python,nodejs} = "my-instrumentation" // -injects to first or all? 
instrumentation.opentelemetry.io/inject-{java,python,nodejs}/{container-name} = "my-instrumentation"

Here, instrumentation.opentelemetry.io/inject-{java,python,nodejs} conteptually is both a leaf value and a grouping of subordinate values, which looks wrong to me, but that is very subjective.

I also don't think we should do dynamic annotation keys to pass container-names at minimum because it would turn processing the annotations of a pod from O(1) to O(n).

I'd instead go with (java as example)

# Triggers injection of java sdk on this pod based on "my-instrumentation"
instrumentation.opentelemetry.io/inject-java = "my-instrumentation" // inject first/all based on field in "my-instrumentation" if no container-names given 
# Optionally, control which container(s) to inject into
instrumentation.opentelemetry.io/inject-java-container-names = "{container-name}[,{container-name}...]"

This does however sacrifice the ability to inject two different instrumentations for one language, however I'm still not sure where that would be needed outside massive breaking API changes, which might be better represented as e.g. "java-v2" instead, which allows for

instrumentation.opentelemetry.io/inject-java
instrumentation.opentelemetry.io/inject-java-v2 

to coexist and to be assigned to different sets of containers

[BronzeDeer]

Oh and Kubernetes actually forbids a second slash in the annotation key: https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set

[BronzeDeer]

@pavolloffay I think this got buried over the holidays, can we pick up where we left of?

[pavolloffay]

Absolutely, let's move this forward ❤️

This does however sacrifice the ability to inject two different instrumentations for one language.

I am maybe missing something but the following could be supported on a single podspec as well.

instrumentation.opentelemetry.io/inject-java = "my-instrumentation" 
instrumentation.opentelemetry.io/inject-java-container-names = "spring-petclinic"
instrumentation.opentelemetry.io/inject-nodejs = "my-instrumentation" 
instrumentation.opentelemetry.io/inject-java-container-names = "nodejs-server"

[BronzeDeer]

@pavolloffay Agreed, I meant having two containers of the same language (say Java) be served by two different Instrumentation CRs. But as I already said, I can't see that happening unless there has been a major api breakage on the OTEL side (say v1 and v2 of Java instrumentation), but in that case I would just go with creating a new "language" as I already outlined.

So, are we agreed on the general approach and final syntax?

[pavolloffay]

be served by two different Instrumentation CRs.

something to document :)

It looks good to me. @anuraaga you were looking into this as well, any concerns?

[anuraaga]

LGTM thanks

[BronzeDeer]

Okay, I'l continue work on the PR for this

[yuriolisa]

@pavolloffay, are we able to close this PR?

[pavolloffay]

This is not a PR :)

The issue is not resolved as far as I can tell.

[yuriolisa]

Ok :D

[jaronoff97]

I believe this PR closed this :)