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 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
+36 −0
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions content/en/docs/languages/js/getting-started/nodejs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

{{% /alert %}}

## Example Application

The following example uses a basic [Express](https://expressjs.com/)
Expand Down Expand Up @@ -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


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.
Comment on lines +256 to +258
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.


```console
$ 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.)

Open <http://localhost:8080/rolldice> in your web browser and reload the page a
few times. After a while you should see the spans printed in the console by the
`ConsoleSpanExporter`.
Expand Down
4 changes: 4 additions & 0 deletions content/en/docs/languages/js/instrumentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

{{% /alert %}}

### Dependencies {#example-app-dependencies}

Create an empty NPM `package.json` file in a new directory:
Expand Down
5 changes: 5 additions & 0 deletions content/en/docs/languages/js/libraries.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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?

{{% /alert %}}

### Setup

Each instrumentation library is an NPM package. For example, here’s how you can
Expand Down
4 changes: 4 additions & 0 deletions content/en/docs/languages/js/propagation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

{{% /alert %}}

## Automatic context propagation

[Instrumentation libraries](../libraries/) like
Expand Down
4 changes: 4 additions & 0 deletions content/en/docs/languages/js/resources.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

{{% /alert %}}

## Process & Environment Resource Detection

Out of the box, the Node.js SDK detects [process and process
Expand Down
4 changes: 4 additions & 0 deletions content/en/docs/languages/js/serverless.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

{{% /alert %}}

## AWS Lambda

{{% alert title="Note" color="info" %}}
Expand Down
Loading