[undici] add all headers

[mbrevda]

Is your feature request related to a problem? Please describe

Headers must be whitelisted to be added to the trace

Describe the solution you'd like to see

Describe alternatives you've considered

Additional context

An option should be added to allow adding all headers, with perhaps a blacklist option of headers not to add (such as sensitive headers)

Thanks!

[pichlermarc]

cc @david-luna @trentm (owners)

[david-luna]

Hi @mbrevda

I guess the instrumentation can assume that if there is a block list for headers it should send them all but the ones in the config. The behaviour could be described this way:

• no headers config => send nothing (for backwards compatibility)
• only headersToSpanAttributes present => send only the headers defined there
• only blockHeadersToSpanAttributes present => send all headers but the ones defined in the config
• both properties defined => sends the ones defined in headersToSpanAttributes if not present in blockheadersToSpanAttributes

so the following config would make undici to send all headers but autorization

{
  "blockheadersToSpanAttributes": {
    "requestHeaders": ["authorization"]
  }
}

Maybe allowing to have a function would be more straight forward but IMO this way eventually we can have this configuration via env vars.

Thoughts?

[trentm]

OTel semconv says this about collecting HTTP request and response headers: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#http-client

**[11]:** Instrumentations SHOULD require an explicit configuration of which headers are to be captured. Including all request headers can be a security risk - explicit configuration helps avoid leaking sensitive information.
The `User-Agent` header is already captured in the `user_agent.original` attribute. Users MAY explicitly configure instrumentations to capture them even though it is not recommended.
The attribute value MUST consist of either multiple header values as an array of strings or a single-item array containing a possibly comma-concatenated string, depending on the way the HTTP library provides access to headers.

I'm sorry I didn't note this before you made a PR for this, David.

This doesn't disallow adding a relatively simple config option to collect all headers (like blockHeadersToSpanAttributes or similar), but it discourages it. Thoughts? I am inclined to think twice before adding this functionality. And if we do, then we add a big security warning.

@mbrevda I believe it is possible to get what you want by using the existing requestHook config option. E.g.: this config option passed to getNodeAutoInstrumentations()

            '@opentelemetry/instrumentation-undici': {
                requestHook: (span, req) => {
                    for (let i = 0; i < req.headers.length; i += 2) {
                        span.setAttribute(`http.request.header.${req.headers[i].toLowerCase()}`,
                            req.headers[i+1]);
                    }
                },

And there is a coming responseHook config option (#2356) that will add the ability to do the same for response headers.

Would that suffice for your use case?

[mbrevda]

Yes, that would work. It's unfortunate that spec prohibits a callback function to chose/sanitize the headers at runtime but the above solution can handle that.

Thanks all!

[mbrevda]

Might be worth noting the above solution in the docs. This can be otherwise closed