-
Notifications
You must be signed in to change notification settings - Fork 580
fix(instr-undici): fix user agent extraction and handle of multiple values on headers #2875
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
Changes from 9 commits
8534336
f41050d
c5c1a77
3b18bf8
9a7f8d7
a263b4d
72fee91
96880ea
01c1c86
48ed223
64eaaba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -165,6 +165,50 @@ | |
}); | ||
} | ||
|
||
private parseRequestHeaders(request: UndiciRequest) { | ||
const result = new Map<string, string | string[]>(); | ||
|
||
if (Array.isArray(request.headers)) { | ||
// headers are an array [k1, v2, k2, v2] (undici v6+) | ||
// values could be string or a string[] for multiple values | ||
for (let i = 0; i < request.headers.length; i += 2) { | ||
const key = request.headers[i]; | ||
const value = request.headers[i + 1]; | ||
|
||
// Key should always be a string, but the types don't know that, and let's be safe | ||
if (typeof key === 'string') { | ||
result.set(key.toLowerCase(), value); | ||
} | ||
} | ||
} else if (typeof request.headers === 'string') { | ||
// headers are a raw string (undici v5) | ||
// headers could be repeated in several lines for multiple values | ||
const headers = request.headers.split('\r\n'); | ||
for (const line of headers) { | ||
if (!line) { | ||
continue; | ||
} | ||
const colonIndex = line.indexOf(':'); | ||
if (colonIndex === -1) { | ||
// Invalid header? Probably this can't happen, but again let's be safe. | ||
continue; | ||
} | ||
const key = line.substring(0, colonIndex).toLowerCase(); | ||
const value = line.substring(colonIndex + 1).trim(); | ||
const allValues = result.get(key); | ||
|
||
if (allValues && Array.isArray(allValues)) { | ||
allValues.push(value); | ||
} else if (allValues) { | ||
result.set(key, [allValues, value]); | ||
} else { | ||
result.set(key, value); | ||
} | ||
} | ||
} | ||
return result; | ||
} | ||
|
||
// This is the 1st message we receive for each request (fired after request creation). Here we will | ||
// create the span and populate some atttributes, then link the span to the request for further | ||
// span processing | ||
|
@@ -218,24 +262,13 @@ | |
} | ||
|
||
// Get user agent from headers | ||
let userAgent; | ||
if (Array.isArray(request.headers)) { | ||
const idx = request.headers.findIndex( | ||
h => h.toLowerCase() === 'user-agent' | ||
); | ||
if (idx >= 0) { | ||
userAgent = request.headers[idx + 1]; | ||
} | ||
} else if (typeof request.headers === 'string') { | ||
const headers = request.headers.split('\r\n'); | ||
const uaHeader = headers.find(h => | ||
h.toLowerCase().startsWith('user-agent') | ||
); | ||
userAgent = | ||
uaHeader && uaHeader.substring(uaHeader.indexOf(':') + 1).trim(); | ||
} | ||
const headersMap = this.parseRequestHeaders(request); | ||
const userAgentValues = headersMap.get('user-agent'); | ||
|
||
if (userAgent) { | ||
if (userAgentValues) { | ||
const userAgent = Array.isArray(userAgentValues) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Choosing the last user-agent value is an "interesting thing" to have a comment on, I think. Could point to the |
||
? userAgentValues[userAgentValues.length - 1] | ||
: userAgentValues; | ||
attributes[SemanticAttributes.USER_AGENT_ORIGINAL] = userAgent; | ||
} | ||
|
||
|
@@ -329,27 +362,14 @@ | |
const headersToAttribs = new Set( | ||
config.headersToSpanAttributes.requestHeaders.map(n => n.toLowerCase()) | ||
); | ||
const headersMap = this.parseRequestHeaders(request); | ||
|
||
// headers could be in form | ||
// ['name: value', ...] for v5 | ||
// ['name', 'value', ...] for v6 | ||
const rawHeaders = Array.isArray(request.headers) | ||
? request.headers | ||
: request.headers.split('\r\n'); | ||
rawHeaders.forEach((h, idx) => { | ||
const sepIndex = h.indexOf(':'); | ||
const hasSeparator = sepIndex !== -1; | ||
const name = ( | ||
hasSeparator ? h.substring(0, sepIndex) : h | ||
).toLowerCase(); | ||
const value = hasSeparator | ||
? h.substring(sepIndex + 1) | ||
: rawHeaders[idx + 1]; | ||
|
||
for (const [name, value] of headersMap.entries()) { | ||
if (headersToAttribs.has(name)) { | ||
spanAttributes[`http.request.header.${name}`] = value.trim(); | ||
const attrValue = Array.isArray(value) ? value.join(', ') : value; | ||
spanAttributes[`http.request.header.${name}`] = attrValue.trim(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I don't think the |
||
} | ||
}); | ||
} | ||
} | ||
|
||
span.setAttributes(spanAttributes); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Not for this PR, but https://opentelemetry.io/docs/specs/semconv/http/http-spans/ says that
user_agent.original
is Opt-In. I.e. collecting it should be off by default. We should do that in a separate (breaking) PR change.