zipkin-js icon indicating copy to clipboard operation
zipkin-js copied to clipboard

expressMiddleware.js and default port = 0

Open hakanson opened this issue 9 years ago • 4 comments

The constructor for expressMiddleware takes a default value for port of 0

module.exports = function expressMiddleware({tracer, serviceName = 'unknown', port = 0}) {

Using ConsoleRecorder I see log records like this, where the port is known to be 3000 as part of the HOST header but port=0 when constructing the InetAddress.

Record at (spanId=b1d6a95787bb7f9d, parentId=b1d6a95787bb7f9d, traceId=b1d6a95787bb7f9d): BinaryAnnotation(http.url="http://localhost:3000/")
Record at (spanId=b1d6a95787bb7f9d, parentId=b1d6a95787bb7f9d, traceId=b1d6a95787bb7f9d): ServerRecv()
Record at (spanId=b1d6a95787bb7f9d, parentId=b1d6a95787bb7f9d, traceId=b1d6a95787bb7f9d): LocalAddr(host="InetAddress(10.211.55.2)", port=0)

It seems like the port should be derived from the HOST header when left as its default value.

      tracer.recordBinary('http.url', url.format({
        protocol: req.protocol,
        host: req.get('host'),
        pathname: req.originalUrl
      }));
      tracer.recordAnnotation(new Annotation.ServerRecv());
      tracer.recordAnnotation(new Annotation.LocalAddr({port}));

hakanson avatar Jun 21 '16 19:06 hakanson

It makes sense that it could pick this stuff up. PR welcome! (any input @eirslett ?)

sveisvei avatar Jun 21 '16 20:06 sveisvei

Go ahead, that's a good change, I think! Even better would be if the middleware had access to the server object, with knows which port it is listening on. In some cases, there could be a call like service a -> haproxy -> service b, and the Host header would then have haproxy's port, while the app would listen on some other port. We prefer that the app's port is shown.

eirslett avatar Jun 21 '16 23:06 eirslett

I not a haproxy expert, but doesn't it change the HOST header to the target hostname and port values and require additional config if you want the original values using the standard headers??

option forwardfor
http-request set-header X-Forwarded-Port %[dst_port]
http-request set-header X-Forwarded-Proto https if { ssl_fc }
http-request set-header X-Forwarded-Host %[dst]

hakanson avatar Jun 22 '16 00:06 hakanson

I don't think localEndpoint should be retrieved from the URL instead it is the tracer providing the information for the local endpoint (as in https://github.com/openzipkin/zipkin-go/blob/master/tracer.go#L36) cc @adriancole

jcchavezs avatar Oct 16 '19 10:10 jcchavezs