[Autoinstrumentation Nodejs] Support exporting traces via http using OTEL_EXPORTER_OTLP_PROTOCOL

[atsu85]

Closes #3412

Description:
Adding a feature: When
OTEL_EXPORTER_OTLP_PROTOCOL=http/json
is set, it uses @opentelemetry/exporter-trace-otlp-http instead of
@opentelemetry/exporter-trace-otlp-grpc
that remains the default (but will be used also when OTEL_EXPORTER_OTLP_PROTOCOL=grpc)

Link to tracking Issue(s):

• Resolves: [Autoinstrumentation Nodejs] use http exporter when OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf #3412

Testing:

• I didn't add automated tests (feature is implemented similarly to OTEL_METRICS_EXPORTER that isn't covered by tests either)
• I tested manually, that when setting environment variables either to

OTEL_EXPORTER_OTLP_PROTOCOL=http/protobuf
OTEL_EXPORTER_OTLP_ENDPOINT=https://tempo.myorg.com
...

or

OTEL_EXPORTER_OTLP_PROTOCOL=http/json
OTEL_EXPORTER_OTLP_ENDPOINT=https://tempo.myorg.com
...

then exporting traces worked, and traces could be seen via Grafana Tempo UI

Documentation:

• I didn't add documentation about this feature, similarly to OTEL_METRICS_EXPORTER that isn't documented in this repository.
• Main documentation is language agnostic and outside of this repository: Protocol configuration, that documents environment variable OTEL_EXPORTER_OTLP_PROTOCOL

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: atsu85 / name: Ats Uiboupin (a21f949, a22b2d5)

[atsu85]

@jaronoff97, thanks for quickly approving it!

What happens next? Does it need to be approved by someone else before someone can merge and release it? If so, then who?

I'm not suggesting it should be released on Friday evening, but it would be nice to understand what's needed :)

[jaronoff97]

@atsu85 for safety, i would like someone from the node SIG to look over the change to be sure i'm not missing anything (I reached out in their slack already). If they don't review it by early next week, I'll have another approver check this out and be sure it looks fine at which point i will merge it.

[trentm]

This is incorrect. The OTLP exporter from @opentelemetry/exporter-trace-otlp-http is actually the exporter for http/json, and not for http/proto. Granted the "http" in the package name is certainly misleading. The exporter package for http/proto is @opentelemetry/exporter-trace-otlp-proto.

Seeing the equivalent handling code in @opentelemetry/sdk-node is helpful:
https://github.com/open-telemetry/opentelemetry-js/blob/eb3ca4fb07ee31c62093f5fcec56575573c902ce/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts#L31-L57

Options:

1. Copy the equivalent of that code from the sdk-node package.
2. Or, another option (which may need more discussion, separate from this PR) might be to remove specifying traceExporter to the NodeSDK constructor, and let the NodeSDK handle selecting the right one. This however, then brings up the question of what the default exporter should be, which impacts the Operator configuration.

changing the default exporter to http/proto?

By default the NodeSDK will use the http/proto trace exporter. Over time http/proto has become the default OTLP protocol used by most OTel languages. You can see this somewhat from the YAML configuration for the OTel Operator Instrumentation given here: https://github.com/open-telemetry/opentelemetry-operator/blob/main/README.md#opentelemetry-auto-instrumentation-injection

It sets the default exporter to http://otel-collector:4317, which is gRPC, but then has custom env for most languages to change to http://otel-collector:4318, which is an HTTP endpoint (for receiving either http/proto or http/json).

I haven't been involved much with the OTel Operator, but I wonder if it would consider switching the default for nodejs over to http://otel-collector:4318 as well. I realize that would be a breaking change.

The benefit is that "autoinstrumentation.ts" could just use the NodeSDK defaults, and not have to handle selecting a traceExporter at all. The NodeSDK's envvar handling will do the right thing.

[atsu85]

The OTLP exporter from @opentelemetry/exporter-trace-otlp-http is actually the exporter for http/json, and not for http/proto. Granted the "http" in the package name is certainly misleading

@trentm, thanks for pointing this out and giving good explanation! It turned out that Grafana Tempo that i used, works with both http/json and http/proto when setting

OTEL_EXPORTER_OTLP_ENDPOINT=https://tempo.myorg.com

so i changed the code to support both (I also tested both manually with Tempo).

[atsu85]

I haven't been involved much with the OTel Operator, but I wonder if it would consider switching the default for nodejs over to http://otel-collector:4318 as well. I realize that would be a breaking change.

The benefit is that "autoinstrumentation.ts" could just use the NodeSDK defaults, and not have to handle selecting a traceExporter at all. The NodeSDK's envvar handling will do the right thing.

That's doesn't sound like a bad change to me in long run, but with this PR I'd keep the backwards compatibility, meaning keeping grpc as the default traces exporter as it was.

If maintainers of this project want to do the breaking change, then perhaps with separate PR, so that the breaking change doesn't block releasing this feature.

[jaronoff97]

@atsu85 would you mind opening an issue for changing the default exporter for node?

[atsu85]

would you mind opening an issue for changing the default exporter for node?

I could, but maybe @trentm you want to do it yourself. Otherwise I would just copy-paste the chapter you wrote

changing the default exporter to http/proto?

...

[trentm]

I created #3423 to discuss this separately. Thanks.

[atsu85]

rebased commits on top of latest main branch to avoid outdated branch error

[swiatekm]

Looks like there's some kind of issue with markdown-link-check in the CI. It has nothing to do with this PR, so I'm going to merge in spite of the changelog check failing.

[atsu85]

Looks like there's some kind of issue with markdown-link-check in the CI. It has nothing to do with this PR, so I'm going to merge in spite of the changelog check failing.

yeah, that looked weird, because before rebase all checks succeeded, including changelog check.
Thanks for merging!