Trace & Logs exporters to honour configuration spec for `OTEL_EXPORTER_OTLP_HEADERS`

[david-luna]

trafficstars

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

Some of the exporters are not taking in consideration the environment variable OTEL_EXPORTER_OTLP_HEADERS as is explained in the configuration spec. Instead they're only looking at the specific environment variable for their scope (log, trace or metric) https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/#otel_exporter_otlp_headers

Traces exporters:

• https://github.com/open-telemetry/opentelemetry-js/blob/4655895ba1a3b62d4031b6ef0ddf52c63f611d0c/experimental/packages/exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts#L46
• https://github.com/open-telemetry/opentelemetry-js/blob/4655895ba1a3b62d4031b6ef0ddf52c63f611d0c/experimental/packages/exporter-trace-otlp-http/src/platform/node/OTLPTraceExporter.ts#L46
• https://github.com/open-telemetry/opentelemetry-js/blob/4655895ba1a3b62d4031b6ef0ddf52c63f611d0c/experimental/packages/exporter-trace-otlp-proto/src/platform/node/OTLPTraceExporter.ts#L49C18-L49C51

Logs exporters:

• https://github.com/open-telemetry/opentelemetry-js/blob/4655895ba1a3b62d4031b6ef0ddf52c63f611d0c/experimental/packages/exporter-logs-otlp-proto/src/platform/node/OTLPLogExporter.ts#L54
• https://github.com/open-telemetry/opentelemetry-js/blob/4655895ba1a3b62d4031b6ef0ddf52c63f611d0c/experimental/packages/exporter-logs-otlp-http/src/platform/node/OTLPLogExporter.ts#L47
• https://github.com/open-telemetry/opentelemetry-js/blob/4655895ba1a3b62d4031b6ef0ddf52c63f611d0c/experimental/packages/exporter-logs-otlp-grpc/src/OTLPLogExporter.ts#L46

Describe the solution you'd like

Exporters must also get the headers from the environment variable OTEL_EXPORTER_OTLP_HEADERS and merge those with the other sources. The spec doe not say anything about priority if there is the same header defined in both vars. IMHO specific environment var for headers (eg. OTEL_EXPORTER_OTLP_LOGS_HEADERS) should have more priority than the generic one within the context of the exporter.

Describe alternatives you've considered

N/A

Additional context

N/A

[pichlermarc]

Hmm, AFAIK this is something we do in the base class. :thinking: There are also tests for this behavior: https://github.com/open-telemetry/opentelemetry-js/blob/4655895ba1a3b62d4031b6ef0ddf52c63f611d0c/experimental/packages/exporter-trace-otlp-http/test/node/CollectorTraceExporter.test.ts#L180-L199

[david-luna]

Thanks for the quick response @pichlermarc :)

I was doing some tests with the User-Agent and I found that behaviour. You're right that the base class gets the headers from OTEL_EXPORTER_OTLP_HEADERS but when it comes to User-Agent header I've spotted this behavior

Given the project from the docs with the following files

/*app.js*/
const express = require('express');

const PORT = parseInt(process.env.PORT || '8080');
const app = express();

function getRandomNumber(min, max) {
  return Math.floor(Math.random() * (max - min) + min);
}

app.get('/rolldice', (req, res) => {
  res.send(getRandomNumber(1, 6).toString());
});

app.listen(PORT, () => {
  console.log(`Listening for requests on http://localhost:${PORT}`);
});

/*instrumentation.js*/
// Require dependencies
const { NodeSDK } = require('@opentelemetry/sdk-node');
const { ConsoleSpanExporter } = require('@opentelemetry/sdk-trace-node');
const {
  getNodeAutoInstrumentations,
} = require('@opentelemetry/auto-instrumentations-node');
const {
  PeriodicExportingMetricReader,
  ConsoleMetricExporter,
} = require('@opentelemetry/sdk-metrics');

const sdk = new NodeSDK({
  // traceExporter: new ConsoleSpanExporter(),
  metricReader: new PeriodicExportingMetricReader({
    exporter: new ConsoleMetricExporter(),
  }),
  instrumentations: [getNodeAutoInstrumentations()],
});

sdk.start();

and the following dependencies

{
  "name": "otel-headers",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@opentelemetry/api": "^1.7.0",
    "@opentelemetry/auto-instrumentations-node": "^0.41.0",
    "@opentelemetry/sdk-metrics": "^1.21.0",
    "@opentelemetry/sdk-node": "^0.48.0",
    "@opentelemetry/sdk-trace-node": "^1.21.0",
    "express": "^4.18.2"
  }
}

when passing User-Agent header to OTEL_EXPORTER_OTLP_TRACES_HEADERS we get the desired header list command:

OTEL_EXPORTER_OTLP_HEADERS="globalkey=val" \
    OTEL_EXPORTER_OTLP_TRACES_HEADERS="globalkey=override,User-Agent=test" \
    node --require ./instrumentation.js app.js

headers received:

{
  "content-type": "application/x-protobuf",
  "globalkey": "override",
  "user-agent": "test",
  "host": "localhost:4318",
  "connection": "close",
  "content-length": "14636"
}

but if adding the same into OTEL_EXPORTER_OTLP_HEADERS var

OTEL_EXPORTER_OTLP_HEADERS="globalkey=val,User-Agent=test" \
    OTEL_EXPORTER_OTLP_TRACES_HEADERS="globalkey=override" \
    node --require ./instrumentation.js app.js

we do get the User-Agent constant from the subclass

{
  "content-type": "application/x-protobuf",
  "globalkey": "override",
  "user-agent": "OTel-OTLP-Exporter-JavaScript/0.48.0",
  "host": "localhost:4318",
  "connection": "close",
  "content-length": "38823"
}

Is that supposed to work that way? Is so, do you think we need to clarify this special case (and others if apply) on the docs?

[pichlermarc]

Ah interesting, I think this is a bug. :bug:

It should actually never allow you override the User-Agent header at all. At least that's how we interpreted this section of the specification: https://opentelemetry.io/docs/specs/otel/protocol/exporter/#user-agent

[pichlermarc]

I can't believe I missed this when reviewing the recent PRs. Here, USER_AGENT should be the last thing we add so that it overrides whatever the user passes in.

[david-luna]

At Elastic we believe there is a use case we might want to control this, distributions https://opentelemetry.io/docs/concepts/distributions/#what-is-a-distribution

In the docs there is a list of what a distribution may include

Customizations in a distribution may include:
- Scripts to ease use or customize use for a specific backend or vendor
- Changes to default settings required for a backend, vendor, or end-user  <-- like USER_AGENT
- Additional packaging options that may be vendor or end-user specific
- Test, performance, and security coverage beyond what OpenTelemetry provides
- Additional capabilities beyond what OpenTelemetry provides
- Less capabilities from what OpenTelemetry provides

We don't think a full override should be available but the possibility to add a prefix to include info about the distribution would come handy so it easy fir the server to identify if the exporter is used by a distro without having to inspect the payloads.

[pichlermarc]

We don't think a full override should be available but the possibility to add a prefix to include info about the distribution would come handy so it easy fir the server to identify if the exporter is used by a distro without having to inspect the payloads.

Hmm, interesting. I'm not sure how other Language SDKs handle this case, so I think this may be an issue do bring up with the specification SIG for clarification.

Personally, I'm not opposed to allowing overrides to User-Agent (it would simplify the implementation too) and I think the use-case you propose is a valid one.

I think that a full override could be the easiest to understand option for everyone:

• user overrides User-Agent -> it does exactly what the user thought it would do
• distro overrides User-Agent -> it does exactly what distro authors want it to do

Whereas the prefix option:

• user overrides User-Agent -> does not what the user thought it would to
• user stumbles upon userAgentPrefix, wonders why it allows a prefix, but not an override

To summarize: I like the full overrides, as it's (IMO) the easiest to understand option. Once there is a statement from the Spec SIG that this overriding the User-Agent is okay, I'd be happy to accept any PR that implements it. :slightly_smiling_face: Maybe this has even already been discussed, but I have not seen it yet :thinking:

[github-actions[bot]]

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.

[github-actions[bot]]

This issue was closed because it has been stale for 14 days with no activity.

[pichlermarc]

re-opening as I think we need to create a follow-up bug issue.

[david-luna]

To summarize: I like the full overrides, as it's (IMO) the easiest to understand option. Once there is a statement from the Spec SIG that this overriding the User-Agent is okay, I'd be happy to accept any PR that implements it. 🙂 Maybe this has even already been discussed, but I have not seen it yet 🤔

From a user perspective I agree with you that a full override is better to understand. As a distro dev I mugh want to keep info about the exporter's user-agent. Could we export that constant in the package?

[pichlermarc]

To summarize: I like the full overrides, as it's (IMO) the easiest to understand option. Once there is a statement from the Spec SIG that this overriding the User-Agent is okay, I'd be happy to accept any PR that implements it. 🙂 Maybe this has even already been discussed, but I have not seen it yet 🤔

From a user perspective I agree with you that a full override is better to understand. As a distro dev I mugh want to keep info about the exporter's user-agent. Could we export that constant in the package?

Hmm, I think I don't follow completely. How would exporting the constant help a distro implementor? :thinking: