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

[Operator] Add docs for instrumenting Deno #5962

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

Conversation

lucacasonato
Copy link

@lucacasonato lucacasonato commented Jan 17, 2025

Also see open-telemetry/opentelemetry-operator#3613

This adds documentation for using the operator to instrument Deno applications by setting OTEL_DENO=true and then injecting with instrumentation.opentelemetry.io/inject-sdk: "true".

cc @jaronoff97


Preview: https://deploy-preview-5962--opentelemetry.netlify.app/docs/kubernetes/operator/automatic/#deno

@svrnm
Copy link
Member

svrnm commented Jan 17, 2025

TIL that deno comes with otel support, wow!

We normally default to not allow documentation for non official otel projects, e.g. we have https://opentelemetry.io/docs/languages/other/ to list 3rd party implementations and language interest (which needs some love and attention from what I just see), but having a language provide out of the box support is very much in line with otel's mission & vision, so we should address that accordingly.

@lucacasonato thank you for bringing this to our attention, we need some time to think about that and address this properly.

cc @open-telemetry/governance-committee @open-telemetry/technical-committee

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

From the operator perspective, the content looks good to me. I'm also very happy with the runtime having first-party Otel support.

Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

🫡

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Copyedit pass. See inline suggestions.

### Deno

The following command creates a basic Instrumentation resource that is
configured for instrumenting Deno services.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
configured for instrumenting Deno services.
configured for instrumenting [Deno](https://deno.com/) services.


{{% alert title="Note" color="info" %}}

Deno's OpenTelemetry integration is not yet stable. As a result all workloads
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Deno's OpenTelemetry integration is not yet stable. As a result all workloads
[Deno's OpenTelemetry integration][deno-otel-docs] is not yet stable. As a result all workloads

Note that I define [deno-otel-docs] further below.

Comment on lines +231 to +233
By default, the Deno OpenTelemetry integration captures `console.log` output as
logs, while still printing the logs to stdout / stderr. There are two other
behaviours that can be configured:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
By default, the Deno OpenTelemetry integration captures `console.log` output as
logs, while still printing the logs to stdout / stderr. There are two other
behaviours that can be configured:
By default, the Deno OpenTelemetry integration exports `console.log()` output as
[logs](/docs/concepts/signals/logs/), while still printing the logs to stdout / stderr. You can configure these alternative behaviors:


{{% /alert %}}

#### Configuring console.log capturing in Deno
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#### Configuring console.log capturing in Deno
#### Configuration options

Comment on lines +235 to +236
- `OTEL_DENO_CONSOLE=replace`: capture `console.log` output as logs and do not
print to stdout / stderr.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `OTEL_DENO_CONSOLE=replace`: capture `console.log` output as logs and do not
print to stdout / stderr.
- `OTEL_DENO_CONSOLE=replace`: only export `console.log()` output as logs; do not
print to stdout / stderr.


- `OTEL_DENO_CONSOLE=replace`: capture `console.log` output as logs and do not
print to stdout / stderr.
- `OTEL_DENO_CONSOLE=ignore`: do not capture `console.log` output as logs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `OTEL_DENO_CONSOLE=ignore`: do not capture `console.log` output as logs.
- `OTEL_DENO_CONSOLE=ignore`: do not export `console.log()` output as logs.

Question: does ignore imply that the output is completely ignored, and hence output isn't printed either? If so then the statement should probably be:

Suggested change
- `OTEL_DENO_CONSOLE=ignore`: do not capture `console.log` output as logs.
- `OTEL_DENO_CONSOLE=ignore`: neither export as logs nor print `console.log()` output.

Comment on lines +241 to +242
For more details, see
[Deno's documentation on the OpenTelemetry integration](https://docs.deno.com/runtime/fundamentals/open_telemetry/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For more details, see
[Deno's documentation on the OpenTelemetry integration](https://docs.deno.com/runtime/fundamentals/open_telemetry/).
For more details, see Deno's [OpenTelemetry integration][deno-otel-docs] documentation.
[deno-otel-docs]: https://docs.deno.com/runtime/fundamentals/open_telemetry/

@@ -684,13 +746,21 @@ Here are a few things to check for:
- **Is the auto-instrumentation for the right language?** For example, when
instrumenting a Python application, make sure that the annotation doesn't
incorrectly say `instrumentation.opentelemetry.io/inject-java: "true"`
instead.
instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this whitespace change? It should be undone, though the formatter should have fixed / reported it.

Comment on lines +756 to +763
{{% alert title="Note" color="info" %}}

Note that unlike other auto-instrumentation, the Deno auto-instrumentation uses
the `instrumentation.opentelemetry.io/inject-sdk: "true"` annotation, not an
annotation that contains the string `deno`.

{{% /alert %}}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather drop this alert and fold this alert text into a sub-bullet of Is the auto-instrumentation for the right language?, which already mentions exceptions (currently for Python). For example, consider replacing the bullet at line 746 of this PR (line 684 in the original file), with:

- **Is the auto-instrumentation for the right language?** For example, when instrumenting an application in:
  - Deno, which uses the `instrumentation.opentelemetry.io/inject-sdk: "true"` annotation, rather than an annotation containing the string `deno`.
  - Python: ensure that the annotation doesn't incorrectly say `instrumentation.opentelemetry.io/inject-java: "true"` instead.

@open-telemetry/docs-approvers WDYT?

@chalin
Copy link
Contributor

chalin commented Jan 17, 2025

Let's keep the sections in alphabetical order: @lucacasonato - could you please move the Go section to be right after the new Deno section?

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

Successfully merging this pull request may close these issues.

5 participants