Add initial support for auto-instrumentation #464

pavolloffay · 2021-10-20T17:29:47Z

Updates #455

This PR adds functionality to inject OpenTelemetry auto-instrumentation (at this point only javaagent) into a pod via admission controller. Note that this is the smallest PR I could submit to support at least something end-to-end. There will be follow up PRs to provide additional features.

Notable changes

new CR to define SDK and instrumentation configuration
javaagent is injected and configured if java.inst.opentelemetry.io/inject: "true" is specified. The value van be the instrumentation CR name.
the javaagent injection uses init container to copy javaagent jar from init container image to a volume that is mounted to the application container
the configuration of the instrumentation and SDK is done via environment variables

CR

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryInst
metadata:
  name: java-instrumentation
spec:
  exporter:
    endpoint: http://otel-collector:1
  java:
    image: ghcr.io/pavolloffay/otel-javaagent:1.5.3

Follow up PRs

Support better service name
Inject k8s resources - namespace, cluster etc.
add sampling configuration to CR
add support for other languages
add status .e.g. show which workloads are instrumented and what is the sampling configuration...
add flag to set default javaagent image
add e2e test

pavolloffay · 2021-10-20T17:32:22Z

@jpkrohling @anuraaga could you please give it a quick look? If it looks fine I will continue and fix tests.

internal/podinjector/main.go

anuraaga

Thanks, this looks great!

anuraaga · 2021-10-21T07:25:51Z

api/v1alpha1/opentelemetryinst_types.go

+	Java JavaSpec `json:"java,omitempty"`
+	// +optional
+	// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true
+	Reporter `json:"reporter,omitempty"`


What does Reporter configure?

It's a structure. It configures everything related to reporting data. At the moment it is only the endpoint.

Is the idea that it's configuration shared among languages rather than language-specific? Maybe it can be called Common or Shared

Yes the idea is to have a common configuration e.g. reporter, sampler at the root level. I am not sure if I like shared or common node in the CR for this, it seems unnecesarilly redundant. I would rather prefer the following (ignore the correctness of the sampler):

apiVersion: opentelemetry.io/v1alpha1 kind: OpenTelemetryInst metadata: name: java-instrumentation spec: reporter: endpoint: http://otel-collector:1 sampler: type: percentrage arg: 0.2 java: image: ghcr.io/pavolloffay/otel-javaagent:1.5.3

Makes sense. In that case let's call it exporter, which is what otel calls this concept.

sounds good.

Shall we think about splitting this into data types? e.g. exporter for traces, metrics? We can start with what we have right now and then add metrics exporter. The current config will server as default and traces one.

internal/podinjector/main.go

pkg/instrumentation/javaagent.go

pavolloffay · 2021-10-21T12:42:36Z

README.md

+    image: ghcr.io/pavolloffay/otel-javaagent:1.5.3 # <1>
+```
+
+1. Docker image with [OpenTelemetry Java auto-instrumentation](https://github.com/open-telemetry/opentelemetry-java-instrumentation). 


cc) @anuraaga this is what the docker image has to execute at runtime

jpkrohling

I couldn't finish reviewing yet but wanted to release my comments already.

README.md

internal/webhookhandler/main.go

jpkrohling · 2021-10-21T12:58:51Z

pkg/instrumentation/annotation.go

+
+const (
+	// annotation contains the annotation name that pods contain, indicating whether a sidecar is desired.
+	annotation = "java.inst.opentelemetry.io/inject"


How about opentelemetry.io/inject-java-autoinstrumentation ?

This is what we have for the sidecar sidecar.opentelemetry.io/inject: "true". Are there any semantics we follow for the annotation name? Here is what istio does https://istio.io/latest/docs/reference/config/annotations/ https://istio.io/latest/docs/reference/config/labels/

I think we should follow something like this:
kind.api-version/functionality

so for the instrumentation it would be instrumentation.opentelemetry.io/inject. Or with the short prefix inst.

Also instead of annotation, I think it's more appropriate to use a label. Here is why:

labels are queriable. The user can get all objects (namespaces, workloads) using the instrumentation feature

annotations are not used to identify and query objects - they are used to store metadata

istio uses a label as well stio-injection=enabled https://istio.io/latest/docs/setup/additional-setup/sidecar-injection/ for sidecar injection

from https://blog.getambassador.io/kubernetes-labels-vs-annotations-95fc47196b6d

Labels are key/value pairs that are attached to objects, such as pods. Labels are intended to be used to specify identifying attributes of objects that are meaningful and relevant to users, but do not directly imply semantics to the core system. Labels can be used to organize and to select subsets of objects. Labels can be attached to objects at creation time and subsequently added and modified at any time … Labels allow for efficient queries and watches and are ideal for use in UIs and CLIs. Non-identifying information should be recorded using annotations.

k8s docs.
https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/

For trivia, I was going to suggest just auto.opentelemetry.io = java or something like that. After looking more at the code I realized that a pod needs to be able to opt out if the namespace opted in, so such a pattern works poorly.

Whether java is in the subdomain or path doesn't have any concrete benefit for me. We do need to decide on one and stick with it going forward though. I don't have enough context on k8s operator naming to pick a side though somehow path does seem more familiar. I can't describe why sorry :(

About labels vs annotations: we use annotations for the user request (please, inject!), and labels to record the state (injected=my-otelcol-instance).

How about instrumentation.opentelemetry.io/inject-javaagent then?

Maybe, instrumentation.opentelemetry.io/inject-java-agent :)

instrumentation.opentelemetry.io/inject-java I think this will do the job. There is only single java instrumentation.

In the future there will be instrumentation.opentelemetry.io/inject-dotnet or instrumentation.opentelemetry.io/inject-sdk to just inject SDK configuration e.g. to serve as a control plane.

pkg/instrumentation/javaagent.go

pavolloffay · 2021-10-21T16:55:40Z

Also the new CRD type is defined in the same package as the collector. If we think they should be in separate packages here is PR #462. First I thought they should be in separate packages so they can evolve separately (e.g. from alpha to v1).

jpkrohling · 2021-10-22T12:11:56Z

That PR has been merged. Click "re-request review" when it's ready.

pavolloffay · 2021-10-25T11:36:43Z

@jpkrohling PR updated and review

jpkrohling · 2021-10-25T13:34:08Z

There are some merge conflicts, possibly due to the rename of the package?

pavolloffay · 2021-10-25T13:59:22Z

rebased again, it should be good now.

api/instrumentation/v1alpha1/opentelemetryinst_types.go

pkg/instrumentation/podmutator.go

pkg/instrumentation/sdk_test.go

jpkrohling · 2021-10-26T09:22:22Z

pkg/sidecar/annotation.go

-// AnnotationValue returns the effective annotation value, based on the annotations from the pod and namespace.
-func AnnotationValue(ns corev1.Namespace, pod corev1.Pod) string {
+// annotationValue returns the effective annotation value, based on the annotations from the pod and namespace.
+func annotationValue(ns corev1.Namespace, pod corev1.Pod) string {


This should still be part of the public API.

There is no need to expose this at this point. The API is not consumed from any other packages.

It is already public. Do you need to make it private for this PR?

jpkrohling · 2021-10-26T09:23:05Z

pkg/sidecar/pod.go

-// Add a new sidecar container to the given pod, based on the given OpenTelemetryCollector.
-func Add(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector, pod corev1.Pod) (corev1.Pod, error) {
+// add a new sidecar container to the given pod, based on the given OpenTelemetryCollector.
+func add(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector, pod corev1.Pod) (corev1.Pod, error) {


Same for these, they should be part of the public API. Are these changes required for this PR?

There is no need to expose this at this point. The API is not consumed from any other packages.

this is called from the pod mutator which has a public API

This is already part of the public API. Please revert this change, unless they are required by your PR.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2021-10-26T11:17:50Z

@jpkrohling PR updated

jpkrohling

LGTM. This is missing an e2e test and docs. Also, what was public API should still be public API, unless you have a great reason to make it private as part of this PR. If it's not required by this PR, I'm open to hearing your arguments in a separate issue.

pkg/instrumentation/sdk_test.go

pavolloffay · 2021-10-26T12:05:10Z

Somebody here is strongly opinionated about using example.com. I would like to know why, the tests do not execute any request.

pavolloffay · 2021-10-26T12:08:01Z

. Also, what was public API should still be public API, unless you have a great reason to make it private as part of this PR. If it's not required by this PR, I'm open to hearing your arguments in a separate issue.

Before this PR those APIs were public bc they were used from the webhook package. This is not the case anymore because whole sidecar injection functionality is in a single well-encapsulated package sidecar. The only exposed API is the Annotation string and mutator implementation for the sidecar.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2021-10-26T18:00:15Z

Added some docs. The e2e test will come in the follow-up PR (See the first comment in this PR with a section for the follow-up work).

jpkrohling · 2021-10-27T09:30:01Z

Somebody here is strongly opinionated about using example.com. I would like to know why, the tests do not execute any request.

When looking at a codebase I don't know and seeing references to endpoints, I'm never sure if it's being mocked somewhere and a call is being made, or if it's just a placeholder. Using example.com makes it clear that this is not a network connection in any way (local, mocked, ...).

jpkrohling

Before this PR those APIs were public bc they were used from the webhook package. This is not the case anymore because whole sidecar injection functionality is in a single well-encapsulated package sidecar. The only exposed API is the Annotation string and mutator implementation for the sidecar.

There are forks of this repository that might be using those public functions (cc @owais). Please, revert those changes to get this merged soon and open an issue proposing making them private. This is a potentially breaking change.

jpkrohling · 2021-10-27T09:35:02Z

docs/otelinst_cr_spec.md

@@ -0,0 +1,20 @@
+# Instrumentation Custom Resource Specification


I appreciate the spirit, but this looks a bit misplaced: it's not linked from anywhere, it's not like anything we have so far for the other CR, and it's not a full example (like the e2e tests or the ones from config/samples. I'm OK in having it, but I'm not sure this is going to be that useful.

you have asked for docs and I thought you are asking for this bc it is in the ./docs directory and there is a similar file for the collector CRD

as mentioned here a couple of times e2e tests will ne done in a follow-up PR. Also I don't want to be adding examples until we have official docker image with javaagent - see #475

pavolloffay · 2021-10-27T12:13:09Z

There are forks of this repository that might be using those public functions (cc @owais). Please, revert those changes to get this merged soon and open an issue proposing making them private. This is a potentially breaking change.

It does not make any sense but let's move it back as you wish. All the functionality is still there just some code moved from two packages to a single package and not it does not have to expose all those APIs that were exposed before due to the previous split design.

Signed-off-by: Pavol Loffay <[email protected]>

anuraaga · 2021-11-01T03:39:22Z

@jpkrohling Just curious if this is ready to get in, with more followups for e.g. e2e testing. Looking forward to extending to add more languages :)

jpkrohling · 2021-11-01T09:45:16Z

I'll try to get a final review later today. If @VineethReddy02 can do it before me and approve, I'll merge it without my final review, assuming you did review the latest version of the PR already, @anuraaga.

jpkrohling

LGTM. Some minor changes to the readme, but nothing serious. I have asked @owais to provide short feedback on the effects of this PR in downstream forks and will merge this later today based on his feedback.

Feel free to work on the readme changes in a follow-up PR.

jpkrohling · 2021-11-02T10:00:53Z

README.md

@@ -146,6 +149,46 @@ spec:
 EOF
 ```

+### OpenTelemetry auto-instrumentation injection
+
+The operator can inject and configure OpenTelemetry auto-instrumentation libraries. At this moment, the operator can inject only


We do not have forced line wraps in this file, as editors can do it as well. While I don't have a particular preference, I do prefer the same style to be used consistently.

README.md

jpkrohling · 2021-11-02T10:13:45Z

@owais just confirmed that this is great from his side as well. Merging!

* Initial support for auto-instrumentation Signed-off-by: Pavol Loffay <[email protected]> * fix Signed-off-by: Pavol Loffay <[email protected]> * Fix bundle Signed-off-by: Pavol Loffay <[email protected]> * Add docs Signed-off-by: Pavol Loffay <[email protected]> * make a nonsense change Signed-off-by: Pavol Loffay <[email protected]> * Add link Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay requested review from a team and dmitryax October 20, 2021 17:29

pavolloffay force-pushed the auto-instrumentation branch 2 times, most recently from 6e512c4 to c4594e4 Compare October 20, 2021 17:54

pavolloffay commented Oct 20, 2021

View reviewed changes

internal/podinjector/main.go Outdated Show resolved Hide resolved

anuraaga reviewed Oct 21, 2021

View reviewed changes

pavolloffay commented Oct 21, 2021

View reviewed changes

jpkrohling reviewed Oct 21, 2021

View reviewed changes

anuraaga mentioned this pull request Oct 25, 2021

Publish a base image containing javaagent open-telemetry/opentelemetry-java-instrumentation#4481

Closed

pavolloffay force-pushed the auto-instrumentation branch from 7cca410 to f01ea0d Compare October 25, 2021 09:49

pavolloffay requested review from jpkrohling, VineethReddy02 and anuraaga October 25, 2021 11:36

pavolloffay force-pushed the auto-instrumentation branch from a03e684 to 2dcf66d Compare October 25, 2021 13:58

jpkrohling reviewed Oct 26, 2021

View reviewed changes

pavolloffay added 2 commits October 26, 2021 13:09

Initial support for auto-instrumentation

7cc730e

Signed-off-by: Pavol Loffay <[email protected]>

fix

e36db4a

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay force-pushed the auto-instrumentation branch from 2dcf66d to e36db4a Compare October 26, 2021 11:16

jpkrohling reviewed Oct 26, 2021

View reviewed changes

Fix bundle

16fc0aa

Signed-off-by: Pavol Loffay <[email protected]>

Add docs

b1600a1

Signed-off-by: Pavol Loffay <[email protected]>

jpkrohling reviewed Oct 27, 2021

View reviewed changes

make a nonsense change

aa12a49

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay force-pushed the auto-instrumentation branch from 70b3577 to aa12a49 Compare October 27, 2021 12:28

Add link

dca837e

Signed-off-by: Pavol Loffay <[email protected]>

anuraaga approved these changes Nov 1, 2021

View reviewed changes

jpkrohling approved these changes Nov 2, 2021

View reviewed changes

jpkrohling merged commit dd248a3 into open-telemetry:main Nov 2, 2021

jpkrohling mentioned this pull request Dec 8, 2021

Remove fmt.Println from test #616

Merged

jpkrohling mentioned this pull request Feb 10, 2022

Promote @pavolloffay to maintainer of the operator #705

Closed

frzifus mentioned this pull request Apr 14, 2022

consider using labels instead of annotations #821

Open

		@@ -0,0 +1,20 @@
		# Instrumentation Custom Resource Specification

Add initial support for auto-instrumentation #464

Add initial support for auto-instrumentation #464

Conversation

pavolloffay commented Oct 20, 2021 • edited Loading

CR

Follow up PRs

pavolloffay commented Oct 20, 2021

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Oct 21, 2021 • edited Loading

jpkrohling commented Oct 22, 2021

pavolloffay commented Oct 25, 2021

jpkrohling commented Oct 25, 2021

pavolloffay commented Oct 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Oct 26, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

pavolloffay commented Oct 26, 2021

pavolloffay commented Oct 26, 2021

pavolloffay commented Oct 26, 2021

jpkrohling commented Oct 27, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Oct 27, 2021 • edited Loading

anuraaga commented Nov 1, 2021

jpkrohling commented Nov 1, 2021 • edited Loading

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Nov 2, 2021

pavolloffay commented Oct 20, 2021 •

edited

Loading

pavolloffay commented Oct 21, 2021 •

edited

Loading

pavolloffay commented Oct 27, 2021 •

edited

Loading

jpkrohling commented Nov 1, 2021 •

edited

Loading