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

david-luna commented 10 months ago

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:

Logs exporters:

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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] commented 8 months ago

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] commented 7 months ago

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

pichlermarc commented 7 months ago

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

david-luna commented 6 months ago

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 commented 5 months ago

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:

github-actions[bot] commented 2 months ago

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.

david-luna commented 2 months ago

Hi @pichlermarc,

the user agent constant contains the current version of the exporter and IMHO I think is a useful info. https://github.com/open-telemetry/opentelemetry-js/blob/2a4919c1cf99d3403d387d7589836fd9e3018896/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/src/OTLPMetricExporter.ts#L37

As a distro developer I want to set the user-agent but keep the exporter version (or even the actual value of UA) as part of the final value.

david-luna commented 2 months ago

After a discussion in 2024-09-25 JS SIG we decided to:

[X] open an issue in otel specifications
[ ] let the full override via env var
[ ] or via code in the exporter, this config could be a function that receives the current UA string and returns a new string

github-actions[bot] commented 1 week ago

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.

open-telemetry / opentelemetry-js