feat(sdk-node): configure trace exporter with environment variables

[svetlanabrennan]

Which problem is this PR solving?

Spec says a user can set an exporter via environment variable with OTEL_TRACES_EXPORTER and specify otlp protocol with OTEL_EXPORTER_OTLP_PROTOCOL or signal specific protocol.

This exporter configuration has been added to the sdk-node package and only applies when the user hasn't set up an exporter or span processor with code (in their tracing.js file).

If a user doesn't set up an exporter in code or a span processor in code or doesn't provide a custom exporter via env vars, the sdk-node will set up a default otlp exporter with a http/protobuf protocol.

Fixes #2873

Short description of the changes

• Added trace exporter env vars and protocol env vars to environment.ts file
• Added code configuring the trace exporter in the sdk-node package in sdk.ts file

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Unit tests
• Integration tests

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

ToDo

• Clarify use of _buildExporterFromEnv() in the BasicTracerProvider

[codecov]

Codecov Report

Merging #3143 (efa2b91) into main (afe0657) will decrease coverage by 0.73%.
The diff coverage is n/a.

❗ Current head efa2b91 differs from pull request most recent head 7db936e. Consider uploading reports for the commit 7db936e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3143      +/-   ##
==========================================
- Coverage   93.36%   92.63%   -0.74%     
==========================================
  Files         240      130     -110     
  Lines        7177     3124    -4053     
  Branches     1487      656     -831     
==========================================
- Hits         6701     2894    -3807     
+ Misses        476      230     -246     

Impacted Files	Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts	90.90% <ø> (ø)
api/src/platform/node/globalThis.ts	0.00% <0.00%> (-100.00%)	⬇️
...s/opentelemetry-core/src/platform/node/sdk-info.ts	0.00% <0.00%> (-100.00%)	⬇️
...opentelemetry-core/src/platform/node/globalThis.ts	0.00% <0.00%> (-100.00%)	⬇️
...pentelemetry-core/src/platform/node/performance.ts	0.00% <0.00%> (-100.00%)	⬇️
.../packages/api-logs/src/platform/node/globalThis.ts	0.00% <0.00%> (-100.00%)	⬇️
...emetry-api-metrics/src/platform/node/globalThis.ts	0.00% <0.00%> (-100.00%)	⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts	31.81% <0.00%> (-68.19%)	⬇️
...perimental/packages/otlp-exporter-base/src/util.ts	79.41% <0.00%> (-17.65%)	⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	84.48% <0.00%> (-15.52%)	⬇️
... and 113 more

[svetlanabrennan]

I have a question I wanted to ask @open-telemetry/javascript-approvers and @open-telemetry/javascript-maintainers before I can change this draft pr to ready for review.

I want to remove the following code since it looks like it was an older attempt at configuring exporters from env:

1. _buildExporterFromEnv()
2. setting up an active span processor
3. _getSpanExporter()

But I'm not 100% sure if this code is necessary here (especially items 2 and 3 above) and if it'll cause breaking changes if I remove that code. Any info/details you can share with me would be much appreciated!

[legendecas]

I want to remove the following code since it looks like it was an older attempt at configuring exporters from env:

1. _buildExporterFromEnv()
2. setting up an active span processor
3. _getSpanExporter()

But I'm not 100% sure if this code is necessary here (especially items 2 and 3 above) and if it'll cause breaking changes if I remove that code. Any info/details you can share with me would be much appreciated!

The (2) is to build a common span processor implementation for spans to send their start/end events. I don't think it should be removed.

(1) and (2) are built upon the registration map _registeredPropagators and _registeredExporters on the subclasses of BasicTracerProvider, like https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts#L38. Is it feasible to configure the OTLP exporter based on that mechanism?

[vmarchaud]

Is it feasible to configure the OTLP exporter based on that mechanism?

Adding on this, this is definitly for this use case that i implemented this back then so that would be cleaner. The way it was supposed to be used is that the sdk extends the node tracer provider with its own exporters/propagators that can then be configured by the env

[svetlanabrennan]

Sorry for the delay on this. I was traveling for work.

I've refactored the code to set up the span processors in the TracerProviderWithEnvExporters constructor and updated/added tests (as mentioned in the feedback above).

However, I have a question regarding this code:

1. Am I using the NoopSpanProcessor correctly here?

I'm trying to only call register when/if TracerProviderWithEnvExporters or NodeSDK set up any span processors and there is a situation in the TracerProviderWithEnvExporters where there are no span processors set up if a user provided incorrect env values or they chose not set up any exporters. In that case, a NoopSpanProcessor is setup (in the inherited BasicTracerProvider class I believe). If I call register without using an if statement then some of existing tests setup in the past don't pass such as the checks around the context manager found here.

2. Is it worth adding a comment above line 216 that was mentioned in item 1 above that explains the purpose of checking if a tracer provide has an NoopSpanProcessor?

[pichlermarc]

I've been looking into the code in a bit more detail regarding your questions @svetlanabrennan 🙂

As far as I can see, we could override the register() method in the TracerProviderWithEnvExporters class. Then, we can only call the superclass's register() method when SpanProcessor have been added. We can also override the addSpanProvider() method, call super.addSpanProvider() to keep track of any calls to that method.

  // part of TracerProviderWithEnvExporters

  override addSpanProcessor(spanProcessor: SpanProcessor) {
    super.addSpanProcessor(spanProcessor);
    this._hasSpanProcessors = true;
  }

  override register(config?: SDKRegistrationConfig) {
    if (this._hasSpanProcessors) {
      super.register(config);
    }
  }

If I am not mistaken, this way it should be possible to call register() without checking for NoopSpanProcessor. 🙂

[svetlanabrennan]

I've been looking into the code in a bit more detail regarding your questions @svetlanabrennan 🙂

As far as I can see, we could override the register() method in the TracerProviderWithEnvExporters class. Then, we can only call the superclass's register() method when SpanProcessor have been added. We can also override the addSpanProvider() method, call super.addSpanProvider() to keep track of any calls to that method.

  // part of TracerProviderWithEnvExporters

  override addSpanProcessor(spanProcessor: SpanProcessor) {
    super.addSpanProcessor(spanProcessor);
    this._hasSpanProcessors = true;
  }

  override register(config?: SDKRegistrationConfig) {
    if (this._hasSpanProcessors) {
      super.register(config);
    }
  }

If I am not mistaken, this way it should be possible to call register() without checking for NoopSpanProcessor. 🙂

This is great feedback. This does remove the need to check for the NoopSpanProcessor. Thank you.

I think just overriding the register method will work and no need to override the addSpanProcessor method because I can use the spanProcessors property to check if any span processors were added before calling register if a selected tracer provider is TracerProviderWithEnvExporters, like this:

// in the TracerProviderWithEnvExporters
  override register(config?: SDKRegistrationConfig) {
    if (this._spanProcessors) {
      super.register(config);
    }
  }

[pichlermarc]

Thank you for working on this and addressing the comments. 🙂
This is looking great. 🎉

[pichlermarc]

The original intent of suggesting to override the addSpanProcessor() method too and keeping track of calls, was to future-proof this in case this class is ever exported from this package. 🙂 As addSpanProcessor() is public, it could be called outside of the constructor. Then registration would not take place when register() is called. 🤔

This is non-blocking for me though 🙂

[svetlanabrennan]

That's fine with me. It's a simple change and future-proof like you said. :)

[legendecas]

Overall LGTM. Thank you for working on this! Just a small nit.