Skip to content
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

[WIP] auto-instrumentation: Allow per container control #683

Conversation

BronzeDeer
Copy link

@BronzeDeer BronzeDeer commented Jan 31, 2022

NOTE: Due to the changes in injection behaviour I have only included the javaagent changes for early review. If the java changes are greenlit, I will add python and nodejs accordingly

fixes #618
fixes #529

Currently, it is only possible to inject a single autoinstrumentation
per pod and it will always be injected into the first container and
first container only. This is especially troublesome if a mutating
webhook will be injecting additional sidecars (e.g. Istio) which will
often receive the instrumentation instead of the main container.
Furthermore, for multi-container, multi-language pods only one container
can currently be auto-instrumented.

This commit introduces a new set of annotations of the form
instrumentation.opentelemetry.io/inject-<language>-container-names,
which tells the operator which containers should receive which language
instrumentation. The value of the annotation should be a comma-delimited
list of container-names to inject into. If the annotation is not
present, injection will use the default container-selection (currently
first container). The annotation will be ignored if the corresponding
language annotation instrumentation.opentelemetry.io/inject-<language>
is not present or set to enable instrumentation for <language>.

Note: It is now possible that instrumentation for multiple languages
needs to be prepared for the same pod. To avoid clashes, the volumes,
volume mount paths and initContainers for each language have been
postfixed with -<language>. Furthermore, to avoid ballooning the
injection logic complexity, the initContainer and volume for a language
will always be added to the Pod as long as injection for that language
is enabled, even if the injection into every container fails.
Previously, with only one language and one container to inject the
operator would only inject the volume and initContainer if the container
could successfully be modified.

Signed-off-by: Joel Pepper [email protected]

fixes open-telemetry#618
fixes open-telemetry#529

Currently, it is only possible to inject a single autoinstrumentation
per pod and it will always be injected into the first container and
first container only. This is especially troublesome if a mutating
webhook will be injecting additional sidecars (e.g. Istio) which will
often receive the instrumentation instead of the main container.
Furthermore, for multi-container, multi-language pods only one container
can currently be auto-instrumented.

This commit introduces a new set of annotations of the form
"instrumentation.opentelemetry.io/inject-<language>-container-names",
which tells the operator which containers should receive which language
instrumentation. The value of the annotation should be a comma-delimited
list of container-names to inject into. If the annotation is not
present, injection will use the default container-selection (currently
first container). The annotation will be ignored if the corresponding
language annotation "instrumentation.opentelemetry.io/inject-<language>"
is not present or set to enable instrumentation for <language>.

Note: It is now possible that instrumentation for multiple languages
needs to be prepared for the same pod. To avoid clashes, the volumes,
volume mount paths and initContainers for each language have been
postfixed with "-<language>". Furthermore, to avoid ballooning the
injection logic complexity, the initContainer and volume for a language
will always be added to the Pod as long as injection for that language
is enabled, even if the injection into every container fails.
Previously, with only one language and one container to inject the
operator would only inject the volume and initContainer if the container
could successfully be modified.

Signed-off-by: Joel Pepper <[email protected]>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 31, 2022

CLA Signed

The committers are authorized under a signed CLA.

@BronzeDeer
Copy link
Author

The changes around volumes and initContainers required updates to the existing tests, I took the liberty to also replace the existing string literals with the corresponding const variables to make the tests less brittle to future changes in names or paths

@BronzeDeer
Copy link
Author

On the topic of tests: It seems there is a lot of duplication in the tests of the individual _test, podmutator_test and sdk_test. When I add tests for the new cases, where should I add which cases?

@BronzeDeer
Copy link
Author

@pavolloffay could you give me a quick feedback on the test question and change in injection behaviour (see above) so I can move this PR along? We've even had duplicate work start croping up, since this stuck in limbo

@BronzeDeer
Copy link
Author

Currently on hold in favor of #689 which implements the simpler special case, see discussion in #689 (comment). Reopen if need for the more general solution arises.

@BronzeDeer BronzeDeer closed this Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant