Add autoinstrumentation of NodeJS #507

anuraaga · 2021-11-05T08:00:19Z

anuraaga · 2021-11-05T08:02:08Z

I think this is basically code complete for a first version, I will add docs but want to confirm the approach for supporting multiple languages. I went with a language annotation as it seemed the most straight forward among a few options I thought of such as changing the language specs to pointers and apply all specs that are defined (assuming there should only be one in practice). But let me know if this makes sense or of any better ideas.

Also this is my first significant change - I was able to verify things fine with a normal app on minikube, but currently when I run USE_EXISTING_CLUSTER make test I get many cgo-type of undecipherable crashes (maybe because of MacOS Monterey?) so will need to figure out how to check those.

jpkrohling · 2021-11-05T09:03:55Z

I'm really excited about this one, thank you for the PR!

There are a couple of tests failing:

=== RUN   TestMutatePod/missing_language
    podmutator_test.go:450: 
        	Error Trace:	podmutator_test.go:450
        	Error:      	Received unexpected error:
        	            	instrumentation.opentelemetry.io/language must be set to the desired SDK language
        	Test:       	TestMutatePod/missing_language
=== RUN   TestMutatePod/missing_annotation
=== RUN   TestMutatePod/annotation_set_to_false
=== RUN   TestMutatePod/annotation_set_to_non_existing_instance
    podmutator_test.go:453: 
        	Error Trace:	podmutator_test.go:453
        	Error:      	"instrumentation.opentelemetry.io/language must be set to the desired SDK language" does not contain "instrumentations.opentelemetry.io \"doesnotexists\" not found"
        	Test:       	TestMutatePod/annotation_set_to_non_existing_instance
--- FAIL: TestMutatePod (0.07s)
    --- PASS: TestMutatePod/javaagent_injection,_true (0.03s)
    --- PASS: TestMutatePod/nodejs_injection,_true (0.01s)
    --- FAIL: TestMutatePod/missing_language (0.00s)
    --- PASS: TestMutatePod/missing_annotation (0.01s)
    --- PASS: TestMutatePod/annotation_set_to_false (0.01s)
    --- FAIL: TestMutatePod/annotation_set_to_non_existing_instance (0.00s)

As the original author of the auto-instrumentation for Java, I'll let @pavolloffay review this one.

pavolloffay

I would like to know why the annotations changes. Could you do this change in a separate PR?

pavolloffay · 2021-11-05T11:56:55Z

pkg/instrumentation/annotation.go

@@ -23,15 +23,16 @@ import (
 const (
 	// annotationInjectJava indicates whether java auto-instrumentation should be injected or not.
 	// Possible values are "true", "false" or "<Instrumentation>" name.
-	annotationInjectJava = "instrumentation.opentelemetry.io/inject-java"


can you explain the intentions for changing this annotation?

Yeah I'm happy to split out the annotation change if it makes sense.

I found our current, SDK-agnostic (?) sdk.go uses this Java annotation. I need it to be able to support multiple languages and came up with this change. Any idea that could work better?

Could you please explain why we need two annotations?

We can have "instrumentation.opentelemetry.io/inject-java", "instrumentation.opentelemetry.io/inject-node" and then `"instrumentation.opentelemetry.io/inject" - to just inject the basic configuration (users could use the operator as a control plane to manage instrumentations).

I don't think we want to allow multiple languages on the same pod as that shouldn't ever be a use case I think. While we could check the annotations and fail if there are multiple this split seemed to model more directly that only one language can be injected.

I made language required without thinking too much but could definitely remove that to allow the control plane only injection if this otherwise makes sense.

I don't see much value of limiting it to only a single language, quite the opposite.

What I don't like is to have two annotations from the UX perspective.

annotationInject = "instrumentation.opentelemetry.io/inject" annotationLanguage = "instrumentation.opentelemetry.io/language"

I believe these sort of annotations are always copy-pasted or frameworked, so I don't think there is a usability difference between the two patterns. If we think there is a use case for multiple annotations, then it makes sense but for example having both otel.inject=true and otel.inject-java=true on the same pod would be redundant and I think that leads to some cognitive dissonance.

Great to hear everyone's ideas on the matter :)

Discussed with @pavolloffay offline and for now sticking to the current annotation format, but we'll need to address it soon. We can see the awkwardness here

https://github.com/open-telemetry/opentelemetry-operator/pull/507/files#diff-92f790ce871022e5e7de195a2f96235dac6ac3fb04337974f8584c7c020a67cbR48

where common config can be injected from different instrumentations in a surprising way. Having separate annotations for the instrumentation config and what languages to use in the future will hopefully solve that.

+1 let's discuss it in a separate issue. maybe there are other ways we can mitigate the issue

…or into nodejs-autoinstrumentatioon

anuraaga · 2021-11-10T07:37:02Z

Sorry for the churn - I filed a couple of issues for some of the challenges I run into while developing here. But this should finally be ready @pavolloffay @jpkrohling

anuraaga · 2021-11-10T07:06:02Z

README.md

+    - baggage
+    - b3
+  nodejs:
+    image: ghcr.io/open-telemetry/opentelemetry-operator/autoinstrumentation-nodejs:latest


@pavolloffay I didn't document this image like you did in Java since I think it's too complicated to create oneself, at least unless someone asks for that.

I think, that is fine. I do not expect many people providing custom builds.

I hoped that vendors could use this operator with their agent distros in the future.

pavolloffay · 2021-11-10T09:14:44Z

README.md

+
+```yaml
+kubectl apply -f - <<EOF
+apiVersion: opentelemetry.io/v1alpha1


I think we should have only a single instrumentation CR in the readme and then have a bulletoints/section for each language with annotations.

Tweaked the README - hoping to be able to follow up with additional doc nits in another PR since it's been a big struggle keeping up with merge conflicts.

Agree I had to rebase yesterday like 100k times

pavolloffay · 2021-11-10T09:15:36Z

pkg/instrumentation/annotation.go

@@ -23,15 +23,16 @@ import (
 const (
 	// annotationInjectJava indicates whether java auto-instrumentation should be injected or not.
 	// Possible values are "true", "false" or "<Instrumentation>" name.
-	annotationInjectJava = "instrumentation.opentelemetry.io/inject-java"


+1 let's discuss it in a separate issue. maybe there are other ways we can mitigate the issue

pavolloffay · 2021-11-10T09:21:04Z

tests/e2e/instrumentation-nodejs/01-install-app.yaml

+    spec:
+      containers:
+      - name: myapp
+        image: ghcr.io/anuraaga/express-hello-world:latest


We should probably create an application that would call an API on our test apps (java, js) that would result in a trace.
Then report the trace to an inmemory storage and asser it exists (e.g. what we do in the java auto-instrumentation tests).

Yup think we will want this

…or into nodejs-autoinstrumentatioon

pavolloffay · 2021-11-10T11:49:33Z

README.md

-EOF
+  nodejs:
+    image: ghcr.io/open-telemetry/opentelemetry-operator/autoinstrumentation-nodejs:latest
+  EOF


there are empty spaces which should not be there

pavolloffay · 2021-11-10T11:50:33Z

README.md

+instrumentation.opentelemetry.io/inject-nodejs: "true"
+```
+
+The value can be


I think we should DRY this, the semantics for the annotation are the same for all languages, no need to repeat this.

pavolloffay · 2021-11-10T11:51:13Z

README.md

 ```

+The above CR can be queried by `kubectl get otelinst`.
+
 1. Container image with [OpenTelemetry Java auto-instrumentation](https://github.com/open-telemetry/opentelemetry-java-instrumentation). The image must contain the Java agent JAR `/javaagent.jar`, and the operator will copy it to a shared volume mounted to the application container.


Let's delete this and consider it as impl detail for now

jpkrohling

I'm approving based on @pavolloffay's approval.

* Add autoinstrumentation of NodeJS * Drift * Switch to single annotation * Fix merge * Fix * Hurts * Format * Revert autogen * Drift * Revert autogen * autogen * Drift * Add doc * e2e * Less doc * Cleanup

anuraaga requested review from a team and bogdandrutu November 5, 2021 08:00

anuraaga marked this pull request as draft November 5, 2021 08:00

anuraaga force-pushed the nodejs-autoinstrumentatioon branch from 4c8896a to fc367e1 Compare November 5, 2021 08:05

Add autoinstrumentation of NodeJS

ba0bede

anuraaga force-pushed the nodejs-autoinstrumentatioon branch from fc367e1 to ba0bede Compare November 5, 2021 08:05

anuraaga mentioned this pull request Nov 5, 2021

Add docker image containing nodejs auto instrumentation #508

Merged

Drift

8a3770a

anuraaga force-pushed the nodejs-autoinstrumentatioon branch from 7461be9 to 8a3770a Compare November 5, 2021 08:32

jpkrohling requested review from pavolloffay and removed request for bogdandrutu November 5, 2021 09:04

pavolloffay reviewed Nov 5, 2021

View reviewed changes

Anuraag Agrawal added 7 commits November 9, 2021 17:17

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

c54a5bc

…or into nodejs-autoinstrumentatioon

Switch to single annotation

ae74544

Fix merge

3c5c95e

Fix

21d5433

Hurts

9b7a81e

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

b0239d6

…or into nodejs-autoinstrumentatioon

Format

7cc5182

anuraaga force-pushed the nodejs-autoinstrumentatioon branch 2 times, most recently from bf8bc85 to 0636df0 Compare November 9, 2021 09:33

Revert autogen

fb23588

anuraaga force-pushed the nodejs-autoinstrumentatioon branch 2 times, most recently from 6e0ad24 to b36d889 Compare November 9, 2021 11:37

Drift

a302e31

anuraaga force-pushed the nodejs-autoinstrumentatioon branch from b36d889 to a302e31 Compare November 9, 2021 12:48

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

1bbe523

…or into nodejs-autoinstrumentatioon

Anuraag Agrawal added 3 commits November 10, 2021 12:20

Revert autogen

293a8e0

autogen

b02d1d9

Drift

0b72fa3

anuraaga force-pushed the nodejs-autoinstrumentatioon branch from 438fc57 to 0b72fa3 Compare November 10, 2021 06:42

Anuraag Agrawal added 2 commits November 10, 2021 16:01

Add doc

50785ae

e2e

cff5521

anuraaga force-pushed the nodejs-autoinstrumentatioon branch from 0a2c238 to cff5521 Compare November 10, 2021 07:16

anuraaga marked this pull request as ready for review November 10, 2021 07:36

anuraaga commented Nov 10, 2021

View reviewed changes

anuraaga mentioned this pull request Nov 10, 2021

Create Docker image for Python auto-instrumentation. #524

Merged

pavolloffay reviewed Nov 10, 2021

View reviewed changes

Anuraag Agrawal added 2 commits November 10, 2021 19:52

Merge branch 'main' of github.com:open-telemetry/opentelemetry-operat…

d063917

…or into nodejs-autoinstrumentatioon

Less doc

d51b0aa

pavolloffay approved these changes Nov 10, 2021

View reviewed changes

Cleanup

b4b8cbb

jpkrohling enabled auto-merge (squash) November 10, 2021 12:48

jpkrohling approved these changes Nov 10, 2021

View reviewed changes

jpkrohling merged commit 15b2f70 into open-telemetry:main Nov 10, 2021

anuraaga mentioned this pull request Nov 11, 2021

Handle case where multiple instrumentations can be applied to a pod #529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add autoinstrumentation of NodeJS #507

Add autoinstrumentation of NodeJS #507

anuraaga commented Nov 5, 2021 •

edited

Loading

anuraaga commented Nov 5, 2021 •

edited

Loading

jpkrohling commented Nov 5, 2021

pavolloffay left a comment

pavolloffay Nov 5, 2021

anuraaga Nov 5, 2021

pavolloffay Nov 5, 2021

anuraaga Nov 6, 2021

pavolloffay Nov 8, 2021

anuraaga Nov 8, 2021

anuraaga Nov 9, 2021

pavolloffay Nov 10, 2021

anuraaga commented Nov 10, 2021

anuraaga Nov 10, 2021

pavolloffay Nov 10, 2021

pavolloffay Nov 10, 2021

anuraaga Nov 10, 2021 •

edited

Loading

pavolloffay Nov 10, 2021

pavolloffay Nov 10, 2021

pavolloffay Nov 10, 2021

anuraaga Nov 10, 2021

pavolloffay Nov 10, 2021

pavolloffay Nov 10, 2021

pavolloffay Nov 10, 2021

jpkrohling left a comment

Add autoinstrumentation of NodeJS #507

Add autoinstrumentation of NodeJS #507

Conversation

anuraaga commented Nov 5, 2021 • edited Loading

anuraaga commented Nov 5, 2021 • edited Loading

jpkrohling commented Nov 5, 2021

pavolloffay 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

Choose a reason for hiding this comment

anuraaga commented Nov 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga Nov 10, 2021 • edited Loading

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

anuraaga commented Nov 5, 2021 •

edited

Loading

anuraaga commented Nov 5, 2021 •

edited

Loading

anuraaga Nov 10, 2021 •

edited

Loading