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

Add clarification for otel-js apps written in CJS and ESM #5072

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JamieDanielson
Copy link
Member

Node.js has two module systems: CommonJS(CJS) and ECMAScript modules(ESM). As ESM becomes more common in usage, we need to better document the current assumptions and requirements.

This is a start with the bare minimum, which is to specify the assumption that the app is running as CJS... as well as document the loader hook currently required for ESM apps. It seemed useful to start here based on issues like #4812.

More details will follow as we finalize reference docs in the otel-js repo.

@JamieDanielson JamieDanielson requested a review from a team August 20, 2024 21:14
@opentelemetrybot opentelemetrybot requested a review from a team August 20, 2024 21:15
$ node --experimental-loader=@opentelemetry/instrumentation/hook.mjs --require ./instrumentation.js app.js
Listening for requests on http://localhost:8080
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Having the h3 section header for this unintentionally puts the subsequent content ("Open http://localhost:8080/rolldice in your web browser ...") under this section.

I don't have a great alternative suggestion. Perhaps the section could be replaced with a much shorter:

(Note: If your application is written in JavaScript as ECMAScript Modules (ESM), or compiled to ESM from TypeScript, then a loader hook is required to properly support instrumentation. Use node --experimental-loader=@opentelemetry/instrumentation/hook.mjs --require ./instrumentation.js app.js. See [the coming reference link in opentelemetry-js/docs/] for details on ESM support.)

@@ -19,6 +19,11 @@ For example,
will automatically create [spans](/docs/concepts/signals/traces/#spans) based on
the inbound HTTP requests.

{{% alert title="Note" color="info" %}}
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
If the application runs as ESM, add the loader hook as specified in [Getting Started](/docs/languages/js/getting-started/nodejs/#instrumentation-for-ecmascript-modules).
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah... in my suggestion above, I've removed this anchor. Perhaps the "see here for more info" could be the opentelemetry-js/doc/ document that is being added in open-telemetry/opentelemetry-js#4876 instead?

Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

Thanks! Some comments.

@@ -24,6 +24,10 @@ Ensure that you have the following installed locally:
- [TypeScript](https://www.typescriptlang.org/download), if you will be using
TypeScript.

{{% alert title="Note" color="info" %}}
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
The OpenTelemetry documentation assumes that the compiled application is run as CommonJS.

Would it make sense to add a link to CommonJS? Are all Node developers familiar with it?

@@ -247,6 +251,17 @@ Listening for requests on http://localhost:8080

{{% /tab %}} {{< /tabpane >}}

### Instrumentation for ECMAScript Modules
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Instrumentation for ECMAScript Modules
### Instrumentation for ECMAScript modules

Comment on lines +256 to +258
If your application is written in JavaScript as ESM, or compiled to ESM from TypeScript, then a loader hook is required to properly patch instrumentation.
The custom hook for ESM instrumentation is `--experimental-loader=@opentelemetry/instrumentation/hook.mjs`.
This flag must be passed to the `node` binary, which is often done as a startup command and/or in the `NODE_OPTIONS` environment variable.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If your application is written in JavaScript as ESM, or compiled to ESM from TypeScript, then a loader hook is required to properly patch instrumentation.
The custom hook for ESM instrumentation is `--experimental-loader=@opentelemetry/instrumentation/hook.mjs`.
This flag must be passed to the `node` binary, which is often done as a startup command and/or in the `NODE_OPTIONS` environment variable.
If your application is written in JavaScript as ESM, or compiled to ESM
from TypeScript, a loader hook is required to patch instrumentation.
The custom hook for ESM instrumentation is
`--experimental-loader=@opentelemetry/instrumentation/hook.mjs`.
This flag must be passed to the `node` binary, which is often done
as a startup command or through the `NODE_OPTIONS` environment variable.

@@ -19,6 +19,11 @@ For example,
will automatically create [spans](/docs/concepts/signals/traces/#spans) based on
the inbound HTTP requests.

{{% alert title="Note" color="info" %}}
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
The OpenTelemetry documentation assumes that the compiled application is run as CommonJS.

@@ -33,6 +33,10 @@ about manual instrumentation.
You don't have to use the example app: if you want to instrument your own app or
library, follow the instructions here to adapt the process to your own code.

{{% alert title="Note" color="info" %}}
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
The OpenTelemetry documentation assumes that the compiled application is run as CommonJS.

@@ -7,6 +7,10 @@ cSpell:ignore: rolldice

{{% docs/languages/propagation js %}}

{{% alert title="Note" color="info" %}}
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
The OpenTelemetry documentation assumes that the compiled application is run as CommonJS.

@@ -15,6 +15,10 @@ Node.js SDK.
Follow the instructions in the [Getting Started - Node.js][], so that you have the
files `package.json`, `app.js` and `tracing.js`.

{{% alert title="Note" color="info" %}}
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
The OpenTelemetry documentation assumes that the compiled application is run as CommonJS.

@@ -8,6 +8,10 @@ cSpell:ignore: otelwrapper
This guide shows how to get started with tracing serverless functions using
OpenTelemetry instrumentation libraries.

{{% alert title="Note" color="info" %}}
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Much of OpenTelemetry JS documentation is written assuming the compiled application is run as CommonJS.
The OpenTelemetry documentation assumes that the compiled application is run as CommonJS.

@JamieDanielson JamieDanielson marked this pull request as draft August 21, 2024 22:58
@JamieDanielson
Copy link
Member Author

I've moved this back to draft for now! It will be easier when we can link to another doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

3 participants