lib: add tracing support for http2 #266
Conversation
RafaelGSS
left a comment
RafaelGSS
left a comment
There was a problem hiding this comment.
I don't have much context to review this in-depth, but it seems very well-tested. Just left a couple of questions, code LGTM.
| const onServerStreamFinishChannel = dc.channel('http2.server.stream.finish'); | ||
| const onServerStreamCloseChannel = dc.channel('http2.server.stream.close'); | ||
|
|
||
| const clientTracingChannels = dc.tracingChannel('http2.client'); |
There was a problem hiding this comment.
| const clientTracingChannels = dc.tracingChannel('http2.client'); | |
| const clientTracingChannels = dc.tracingChannel('http2.client'); |
Should we have a http2.server.session too?
There was a problem hiding this comment.
We could, but at the moment I'm only tracking streams (http transactions). We could of course add support for sessions (to track actual connections). We can add those later on. At least this way we have the same support as we have for http and fetch.
| if (headers[HTTP2_HEADER_METHOD] === HTTP2_METHOD_HEAD) { | ||
| // For head requests, there must not be a body... | ||
| // end the writable side immediately. | ||
| stream.end(); |
There was a problem hiding this comment.
shouldn't it be inside an else if instead? What if we receive an endOfStream = true and HTTP2_HEADER_METHOD = head?
There was a problem hiding this comment.
Maybe, to be honest I've just kept the same logic it was implemented before.
santigimeno
force-pushed
the
santi/diag_http2
branch
from
8b7b524 to
52285f6
Compare
santigimeno
force-pushed
the
santi/diag_http2_tracing
branch
from
c554ce0 to
abbaab0
Compare
santigimeno
changed the base branch from
santi/diag_http2
to
node-v22.x-nsolid-v5.x
santigimeno
force-pushed
the
santi/diag_http2_tracing
branch
from
abbaab0 to
c392d14
Compare
santigimeno
force-pushed
the
santi/diag_http2_tracing
branch
from
c392d14 to
6e50225
Compare
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
|
Landed in 65430e7...ed8c4f9 |
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #268
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #268
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #269
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #269
|
hi folks, are there any plans to upstream this work? i started some research into this last week in nodejs/node#57955, but it looks like you guys are way ahead already |
Hi! Was planning to when I had the bandwidth, but not at this moment so feel free to pick ecfdc05 if it works for you. There are a couple of things missing in that commit to be ready for upstream though:
Also, we haven't added tracing channels for tracking sessions yet, just streams (this could be done separately) |
While also adding tracing channels to track http2 client and server streams lifetime.