-
Notifications
You must be signed in to change notification settings - Fork 487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[undici] add all headers #2226
Comments
cc @david-luna @trentm (owners) |
Hi @mbrevda I guess the instrumentation can assume that if there is a
so the following config would make undici to send all headers but {
"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? |
OTel semconv says this about collecting HTTP request and response 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 @mbrevda I believe it is possible to get what you want by using the existing '@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 Would that suffice for your use case? |
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! |
Might be worth noting the above solution in the docs. This can be otherwise closed |
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!
The text was updated successfully, but these errors were encountered: