NodeSDK support for OTEL_TRACES_EXPORTER, ...

[francisdb]

Is your feature request related to a problem? Please describe.

To consolidate configuration for all our services we would like to configure the node sdk fully through the use of env vars like we do for the java/ruby sdk

see the related specs here:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#otlp-exporter
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md

Describe the solution you'd like

One example would be the following

// import buildTraceExporterFromEnv from somewhere
// configure the exporters you want to allow somewhere?

const sdk = new opentelemetry.NodeSDK({
    resource: new Resource({
        [SemanticResourceAttributes.SERVICE_NAME]: 'myservice',
    }),
    traceExporter: buildTraceExporterFromEnv(),
    instrumentations: [getNodeAutoInstrumentations()]
})

that buildTraceExporterFromEnv could also be the default for traceExporter

In the code we register a number of exporters we want to support

OTEL_TRACES_EXPORTER=otlp
OTEL_EXPORTER_OTLP_TRACES_PROTOCOL=http/protobuf
OTEL_EXPORTER_OTLP_TRACES_ENDPOINT="https://..../v1/traces"

Describe alternatives you've considered

We currently have to do handle the OTEL_TRACES_EXPORTER / OTEL_EXPORTER_OTLP_PROTOCOL to exporter selection manually.

Additional context

Current situation:

• if you don't set a traceExporter on the NodeSDK tracing will be disabled

• When using BasicTracerProvider the exporter is configured through OTEL_TRACES_EXPORTER as in the spec

  opentelemetry-js/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts

  Lines 251 to 261 in 7870e95

  	protected _buildExporterFromEnv(): SpanExporter | undefined {
  	const exporterName = getEnv().OTEL_TRACES_EXPORTER;
  	if (exporterName === 'none') return;
  	const exporter = this._getSpanExporter(exporterName);
  	if (!exporter) {
  	diag.error(
  	`Exporter "${exporterName}" requested through environment variable is unavailable.`
  	);
  	}
  	return exporter;
  	}

[svetlanabrennan]

I'll like to work on this issue please.

[svetlanabrennan]

A few questions on OTEL_TRACES_EXPORTER:

1. This is the only place that I can find that uses OTEL_TRACES_EXPORTER but I don't see it setting up any exporters. From this pr, it looks like it was just the first step to allowing this feature someday. Is my understanding correct? @vmarchaud Since you worked on that pr, your feedback/clarification would be appreciated.

2. Adding OTEL_TRACES_EXPORTER was questioned in the past in various issues (go, js, spec) because adding the different exporters to an SDK as dependencies would make the SDK heavy. So:

   • Do we still want to add this feature? And should we add this feature to the auto instrumenting packages like the sdk-trace-node and/or the sdk-node?
   • If someone sets OTEL_TRACES_EXPORTER to otlp, but didn't set OTEL_EXPORTER_OTLP_TRACES_PROTOCOL what protocol should we use (http/json, http/proto or grpc)?

Thanks for any feedback.

[francisdb]

This is what we currently use but it's far from complete

function setupTraceExporter(): SpanExporter | undefined {
  const exporter = process.env.OTEL_TRACES_EXPORTER ?? 'none'
  switch (exporter) {
    case 'otlp': {
      const protocol =
        process.env.OTEL_EXPORTER_OTLP_TRACES_PROTOCOL ??
        process.env.OTEL_EXPORTER_OTLP_PROTOCOL ??
        'http/protobuf'
      switch (protocol) {
        case 'http/protobuf':
          return new ProtoTraceExporter()
        case 'grpc':
          return new OTLPTraceExporter()
        default:
          throw new Error(`Unsupported OpenTelemetry protocol ${protocol}`)
      }
    }
    case 'console':
      return new opentelemetry.tracing.ConsoleSpanExporter()
    case 'none':
      // we can't use pino here, otherwise it will not be instrumented
      console.log('Tracing disabled.')
      return undefined
    default:
      throw new Error(
        `OpenTelemetry OTEL_TRACES_EXPORTER invalid value: ${exporter}`,
      )
  }
}

[francisdb]

There is a section on the default protocol for otlp:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#specify-protocol

If no configuration is provided the default transport SHOULD be http/protobuf unless ...

[vmarchaud]

Is my understanding correct?

Yes, i originally implemented few exporters in the PR before removing them because the sdk size.

Do we still want to add this feature? And should we add this feature to the auto instrumenting packages like the sdk-trace-node and/or the sdk-node?

From our discussion at the time we agreed that we didn't want this into the sdk-trace-node since its the core sdk that we want to be as lightweight as possible. However i do think this feature has its place into the sdk-node package where we bundle common exporters so end user can easily use them.

If someone sets OTEL_TRACES_EXPORTER to otlp, but didn't set OTEL_EXPORTER_OTLP_TRACES_PROTOCOL what protocol should we use (http/json, http/proto or grpc)?

Doesn't the spec precise a default OTEL_EXPORTER_OTLP_TRACES_PROTOCOL ?

[svetlanabrennan]

Doesn't the spec precise a default OTEL_EXPORTER_OTLP_TRACES_PROTOCOL ?

Ah yes I see it now. @francisdb provided a link to it above - I must have missed. The default is http/protobuf.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[svetlanabrennan]

Still working on this

[Luckz]

I believe these environment variables are (also) useful for the inverse: to have no effect when not provided, and to disable the traces or metrics ( or future logs) exporter that's configured in code.

const sdk = new NodeSDK({
    metricExporter,
    traceExporter,

&

OTEL_METRICS_EXPORTER=none

[UdyW]

Any update on this please @svetlanabrennan. If not could you please assign the issue to someone more capable?

[dyladan]

Any update on this please @svetlanabrennan. If not could you please assign the issue to someone more capable?

Please do not disparage contributors. A small handful of people are spread very thin over a large project. If you want to know the status of an issue you can ask if there is any update but criticizing the ability of contributors will not be tolerated.

[UdyW]

Hi @dyladan,
Thank you for your concern in this matter. This issue was reported 6months ago and assigned to @svetlanabrennan 4months ago. I was just trying to mention him if he needs any support please assign it to someone with more experience. Just trying to help. Sorry if it seems to you a different way.
I believe 4months is enough time to work on a task with this size.

[vmarchaud]

@UdyW As you can see in the issue itself (one comment prior to yours) a PR has been opened one month ago: #3143 and if you check it there has been activity on it last week.

Again, please avoid commenting passive-aggresive thing like someone with more experience and I believe 4months is enough time to work on a task with this size, thanks.