elastic / elastic/apm-agent-nodejs
`context.request.url` does not properly handle http2
- Dominant language
- JavaScript
- Stars
- 594
- Forks
- 244
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 16
Description
`{transaction,span,error}.context.request.url` is set in the agent by `getContextFromRequest` in lib/parsers.js: https://github.com/elastic/apm-agent-nodejs/blob/v3.17.0/lib/parsers.js#L18-L24
This uses the ["original-url"](https://github.com/watson/original-url#readme) package, to extract a URL object (in the form specified by the apm-server JSON specs -- https://github.com/elastic/apm-server/tree/master/docs/spec/v2/) from a node.js http server request object ((`http.IncomingMessage`)[https://nodejs.org/api/http.html#http_class_http_incomingmessage]).
With an http2 server the wrapped ["stream" event](https://nodejs.org/api/http2.html#http2_event_stream) does not have a direct IncomingMessage object. Currently the http2 instrumentation is setting [this artificial req object](https://github.com/elastic/apm-agent-nodejs/blob/v3.17.0/lib/instrumentation/modules/http2.js#L53-L59) on the transaction, which is later passed to `original-url` to create the URL fields. The result is something like `{ raw: '/', protocol: 'http:', pathname: '/' }` (see [this test code](https://github.com/elastic/apm-agent-nodejs/blob/v3.17.0/test/instrumentation/modules/http2.js#L381-L385)) instead of the more typical:
```
{
"raw": "/",
"protocol": "https:",
"hostname": "localhost",
"port": "$port",
"pathname": "/",
"full": "https://localhost:$port/"
},
```
from an https request.
Is that `protocol: 'http:'` wrong here? Shouldn't it be 'https:'?
Here is a smaller sample script that does the equivalent calls for dev testing:
```js
const fs = require('fs')
const http2 = require('http2')
const originalUrl = require('original-url')
const server = http2.createSecureServer({
allowHTTP1: true,
key: fs.readFileSync('localhost-privkey.pem'),
cert: fs.readFileSync('localhost-cert.pem')
})
server.on('stream', (stream, headers) => {
stream.respond({
'content-type': 'text/html; charset=utf-8',
':status': 200
})
const req = {
headers,
socket: stream.session.socket,
method: headers[':method'],
url: headers[':path'],
httpVersion: '2.0'
}
const url = originalUrl(req)
console.warn('XXX url: ', url)
console.warn('XXX from req: ', req)
stream.end(`Original URL: ${url.full}\n`)
})
server.listen(1337)
// 1. Create a local self-signed cert:
// openssl req -x509 -newkey rsa:2048 -nodes -sha256 -subj '/CN=localhost' -keyout localhost-privkey.pem -out localhost-cert.pem
// 2. Run the server:
// node playorigurl.js
// 3. Make an HTTP/2 request (use '-vv' curl option to see ALPN negotiation)
// curl https://localhost:8443 -ki
//
// Also worth testing is an HTTP/1.1 request:
// curl https://localhost:8443 -i --http1.1
```
Perhaps original-url could be updated (or the logic pulled into the agent) to support https://nodejs.org/api/http2.html classes like `Http2Session` and `Http2Stream` or the different HTTP/2 headers and/or the compatibility API classes http2.Http2ServerRequest (which would require instrumenting the "request" event).
```
Contributor guide
Assessment
This issue has not been assessed yet.