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.

Sign up for GitHub

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

Add Node.js support for --import flag #3416

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add Node.js support for --import flag #3416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6bb45bf
Add Node.js support for --import flag
raphael-theriault-swi Nov 1, 2024
30c7f36
Fix tsconfig for Node.js autoinstrumentation
raphael-theriault-swi Nov 1, 2024
f4e5a79
add subtext to changelog
raphael-theriault-swi Nov 4, 2024
3b65d65
fix kustomization
raphael-theriault-swi Nov 5, 2024
4421cc0
Merge branch 'main' into node-import
Nov 6, 2024
993d67f
run reset again
raphael-theriault-swi Nov 6, 2024
555ddcb
address JS import flag review
raphael-theriault-swi Nov 8, 2024
a8e0a47
update docs
raphael-theriault-swi Nov 8, 2024
d6747dd
Merge branch 'main' into node-import
raphael-theriault-swi Nov 21, 2024
da2827e
use spaces in changelog
raphael-theriault-swi Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .chloggen/node-import.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
component: auto-instrumentation

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add Node.js support for `--import` flag

# One or more tracking issues related to the change
issues: [3414]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext: |
The new UseImport option overrides the default injected --require flag with an --import flag.
When using the provided container image, this enables instrumentation of ESM code.
This behavior may be different for other images, and the option may even be either required or unsupported.
Node.js ^18.19.0 || ^20.6.0 || >=22 is required for the flag to be supported.
7 changes: 7 additions & 0 deletions apis/v1alpha1/instrumentation_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,13 @@ type NodeJS struct {
// Resources describes the compute resource requirements.
// +optional
Resources corev1.ResourceRequirements `json:"resourceRequirements,omitempty"`

// UseImport overrides the default injected --require flag with an --import flag.
// When using the provided container image, this enables instrumentation of ESM code.
// This behavior may be different for other images, and the option may even be either required or unsupported.
// Node.js ^18.19.0 || ^20.6.0 || >=22 is required for the flag to be supported.
// +optional
UseImport bool `json:"useImport,omitempty"`
Copy link

@trentm trentm Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One subtlety that might impact the choice of the config var name.
The feature for the user is whether a module import hook is added (via module.register()) to support instrumentating ESM code.

That is actually independent of whether --import something.mjs or --require something.js is used to run the OTel setup when Node.js is starting up. It is definitely possible to call module.register("@opentelemetry/instrumentation/hooks.mjs"); from the CommonJS autoinstrumentation.js (loaded via --require).

So I wonder if a config name something like instrumentEsm or something like that would be clearer.

There are a few subtleties discussed at open-telemetry/opentelemetry-js#4933, but I don't think you need to read and grok all that. Eventually we'd like it so that there is just the one obvious way to setup OTel -- and that would be via --import some-esm-file-that-calls-module.register.mjs. However, for now we need to make the ESM support opt-in because:

  • it requires newer versions of node
  • it is possible the user explicitly does not want ESM instrumentation enabled because it is still new and can have overhead and bugs

Copy link
Author

@raphael-theriault-swi raphael-theriault-swi Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instrumentEsm here would be a bit of a misnomer, given that functionality could be added without an import flag, and a custom image could also require the use of an import flag for top level await support (which is actually our usecase for this addition) without providing ESM instrumentation

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a custom image could also require the use of an import flag for top level await support (which is actually our usecase for this addition)

Ah, I misunderstood the use case then. Using import in the var name makes sense then. The docs for this feature should mention the two things then: (1) --import ... is used to load "autoinstrumentation.mjs" (so custom versions can use ESM and top-level await), (2) the default "autoinstrumentation.mjs" differs from "autoinstrumentation.js" in that it registers the hook for ESM instrumentation.

}

// Python defines Python SDK and instrumentation configuration.
Expand Down
2 changes: 2 additions & 0 deletions autoinstrumentation/nodejs/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
# - Grant the necessary access to `/autoinstrumentation` directory. `chmod -R go+r /autoinstrumentation`
# - For auto-instrumentation by container injection, the Linux command cp is
# used and must be availabe in the image.
# - By default a file named autoinstrumentation.js is loaded using `require`, but the UseImport configuration option
# overrides this default behaviour and loads a file name autoinstrumentation.mjs using `import`.
FROM node:20 AS build

WORKDIR /operator-build
Expand Down
1 change: 1 addition & 0 deletions autoinstrumentation/nodejs/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"@opentelemetry/exporter-metrics-otlp-grpc": "0.54.0",
"@opentelemetry/exporter-prometheus": "0.54.0",
"@opentelemetry/exporter-trace-otlp-grpc": "0.54.0",
"@opentelemetry/instrumentation": "0.54.0",
"@opentelemetry/resource-detector-alibaba-cloud": "0.29.4",
"@opentelemetry/resource-detector-aws": "1.7.0",
"@opentelemetry/resource-detector-container": "0.5.0",
Expand Down
10 changes: 10 additions & 0 deletions autoinstrumentation/nodejs/src/autoinstrumentation.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import "./autoinstrumentation.js";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raphael-theriault-swi how does the --import flag with with the upstream opentelemetry/auto-instrumentations-node ? We are trying to minimize the nodejs code and rely only on that package.

Related ticket: #3465

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trentm could you please take a look here? cc) @JamieDanielson

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavolloffay Are you asking if node --import @opentelemetry/auto-instrumentations-node/register ... works? If so, I think yes it should work fine because node --import something ... works with something being either ESM or CommonJS code. I haven't tried it, though.

However, whether to move the operator to using @opentelemetry/auto-instrumentations-node instead of the bootstrap code in "autoinstrumentation.js" doesn't need to be part of this PR, right?

Copy link
Author

@raphael-theriault-swi raphael-theriault-swi Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add that the instrumentation.js file needs to exist. Adding a --require @opentelemetry/auto-instrumentations-node/register option alone will not work, since this requires the package to be present in the node_modules search path of the application itself, which would require the application to have the package installed and defeat the entire purpose of the operator autoinstrumentation.

The contents of the file could be replaced by require("@opentelemetry/auto-instrumentations-node/register") instead, which would not conflict with this PR whatsoever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, whether to move the operator to using @opentelemetry/auto-instrumentations-node instead of the bootstrap code in "autoinstrumentation.js" doesn't need to be part of this PR, right?

The issue is that, if we keep adding new features here it will be harder to migrate them to auto-instrumentations-node (or other node packages)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I mentioned above switching to auto-instrumentations-node would be not require any change to the code added by this PR now or in the future.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavolloffay Are there any changes you would like to see with regards to the switch auto-instrumentations-node ? I can make that part of the PR, but again either way it wouldn't really change any files this PR touches.

import module from "node:module";

if (typeof module.register === "function") {
module.register("@opentelemetry/instrumentation/hooks.mjs");
} else {
console.warn(
`OpenTelemetry Operator auto-instrumentation could not instrument ESM code: Node.js ${process.version} does not support 'module.register()'`
);
}
3 changes: 2 additions & 1 deletion autoinstrumentation/nodejs/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"forceConsistentCasingInFileNames": true,
"incremental": true,
"inlineSources": true,
"module": "commonjs",
"module": "node16",
"newLine": "LF",
"noEmitOnError": true,
"noFallthroughCasesInSwitch": true,
Expand All @@ -27,6 +27,7 @@
},
"include": [
"src/**/*.ts",
"src/**/*.mts"
],
"exclude": [
"node_modules"
Expand Down
2 changes: 2 additions & 0 deletions bundle/community/manifests/opentelemetry.io_instrumentations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1496,6 +1496,8 @@ spec:
x-kubernetes-int-or-string: true
type: object
type: object
useImport:
type: boolean
volumeClaimTemplate:
properties:
metadata:
Expand Down
2 changes: 2 additions & 0 deletions bundle/openshift/manifests/opentelemetry.io_instrumentations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1496,6 +1496,8 @@ spec:
x-kubernetes-int-or-string: true
type: object
type: object
useImport:
type: boolean
volumeClaimTemplate:
properties:
metadata:
Expand Down
2 changes: 2 additions & 0 deletions config/crd/bases/opentelemetry.io_instrumentations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1494,6 +1494,8 @@ spec:
x-kubernetes-int-or-string: true
type: object
type: object
useImport:
type: boolean
volumeClaimTemplate:
properties:
metadata:
Expand Down
10 changes: 10 additions & 0 deletions docs/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5666,6 +5666,16 @@ If the former var had been defined, then the other vars would be ignored.<br/>
Resources describes the compute resource requirements.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>useImport</b></td>
<td>boolean</td>
<td>
UseImport overrides the default injected --require flag with an --import flag.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this documentation should be expanded a bit. It is relevant that useImport: true also changes to using the autoinstrumentation.mjs file (rather than autoinstrumentation.js). Anyone providing a custom image for nodejs injection should be aware of that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's documented in the Dockerfile which is where the related info about autoinstrumentation.js was, I don't feel strongly about it either way but I wasn't sure whether documenting the internal filenames made sense in the user-facing documentation.

When using the provided container image, this enables instrumentation of ESM code.
This behavior may be different for other images, and the option may even be either required or unsupported.
Node.js ^18.19.0 || ^20.6.0 || >=22 is required for the flag to be supported.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b><a href="#instrumentationspecnodejsvolumeclaimtemplate">volumeClaimTemplate</a></b></td>
<td>object</td>
Expand Down
11 changes: 9 additions & 2 deletions pkg/instrumentation/nodejs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
const (
envNodeOptions = "NODE_OPTIONS"
nodeRequireArgument = " --require /otel-auto-instrumentation-nodejs/autoinstrumentation.js"
nodeImportArgument = " --import /otel-auto-instrumentation-nodejs/autoinstrumentation.mjs"
nodejsInitContainerName = initContainerName + "-nodejs"
nodejsVolumeName = volumeName + "-nodejs"
nodejsInstrMountPath = "/otel-auto-instrumentation-nodejs"
Expand All @@ -47,14 +48,20 @@ func injectNodeJSSDK(nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int) (cor
}
}

var nodeArgument string
if nodeJSSpec.UseImport {
nodeArgument = nodeImportArgument
} else {
nodeArgument = nodeRequireArgument
}
idx := getIndexOfEnv(container.Env, envNodeOptions)
if idx == -1 {
container.Env = append(container.Env, corev1.EnvVar{
Name: envNodeOptions,
Value: nodeRequireArgument,
Value: nodeArgument,
})
} else if idx > -1 {
container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument
container.Env[idx].Value = container.Env[idx].Value + nodeArgument
}

container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Expand Down
114 changes: 114 additions & 0 deletions pkg/instrumentation/nodejs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,120 @@ func TestInjectNodeJSSDK(t *testing.T) {
},
err: fmt.Errorf("the container defines env var value via ValueFrom, envVar: %s", envNodeOptions),
},
{
name: "NODE_OPTIONS not defined and UseImport true",
NodeJS: v1alpha1.NodeJS{Image: "foo/bar:1", UseImport: true},
pod: corev1.Pod{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{},
},
},
},
expected: corev1.Pod{
Spec: corev1.PodSpec{
Volumes: []corev1.Volume{
{
Name: "opentelemetry-auto-instrumentation-nodejs",
VolumeSource: corev1.VolumeSource{
EmptyDir: &corev1.EmptyDirVolumeSource{
SizeLimit: &defaultVolumeLimitSize,
},
},
},
},
InitContainers: []corev1.Container{
{
Name: "opentelemetry-auto-instrumentation-nodejs",
Image: "foo/bar:1",
Command: []string{"cp", "-r", "/autoinstrumentation/.", "/otel-auto-instrumentation-nodejs"},
VolumeMounts: []corev1.VolumeMount{{
Name: "opentelemetry-auto-instrumentation-nodejs",
MountPath: "/otel-auto-instrumentation-nodejs",
}},
},
},
Containers: []corev1.Container{
{
VolumeMounts: []corev1.VolumeMount{
{
Name: "opentelemetry-auto-instrumentation-nodejs",
MountPath: "/otel-auto-instrumentation-nodejs",
},
},
Env: []corev1.EnvVar{
{
Name: "NODE_OPTIONS",
Value: " --import /otel-auto-instrumentation-nodejs/autoinstrumentation.mjs",
},
},
},
},
},
},
err: nil,
},
{
name: "NODE_OPTIONS defined and UseImport true",
NodeJS: v1alpha1.NodeJS{Image: "foo/bar:1", Resources: testResourceRequirements, UseImport: true},
pod: corev1.Pod{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Env: []corev1.EnvVar{
{
Name: "NODE_OPTIONS",
Value: "-Dbaz=bar",
},
},
},
},
},
},
expected: corev1.Pod{
Spec: corev1.PodSpec{
Volumes: []corev1.Volume{
{
Name: "opentelemetry-auto-instrumentation-nodejs",
VolumeSource: corev1.VolumeSource{
EmptyDir: &corev1.EmptyDirVolumeSource{
SizeLimit: &defaultVolumeLimitSize,
},
},
},
},
InitContainers: []corev1.Container{
{
Name: "opentelemetry-auto-instrumentation-nodejs",
Image: "foo/bar:1",
Command: []string{"cp", "-r", "/autoinstrumentation/.", "/otel-auto-instrumentation-nodejs"},
VolumeMounts: []corev1.VolumeMount{{
Name: "opentelemetry-auto-instrumentation-nodejs",
MountPath: "/otel-auto-instrumentation-nodejs",
}},
Resources: testResourceRequirements,
},
},
Containers: []corev1.Container{
{
VolumeMounts: []corev1.VolumeMount{
{
Name: "opentelemetry-auto-instrumentation-nodejs",
MountPath: "/otel-auto-instrumentation-nodejs",
},
},
Env: []corev1.EnvVar{
{
Name: "NODE_OPTIONS",
Value: "-Dbaz=bar" + " --import /otel-auto-instrumentation-nodejs/autoinstrumentation.mjs",
},
},
},
},
},
},
err: nil,
},
}

for _, test := range tests {
Expand Down
Loading
Loading