lib: add tracing support for http2 #266
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RafaelGSS
reviewed
Feb 10, 2025
| @@ -199,6 +199,9 @@ const onServerStreamErrorChannel = dc.channel('http2.server.stream.error'); | |||
| 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.
Suggested change
| 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.
juanarbol
approved these changes
Feb 10, 2025
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
RafaelGSS
approved these changes
Feb 12, 2025
santigimeno
force-pushed
the
santi/diag_http2_tracing
branch
from
c392d14 to
6e50225
Compare
santigimeno
added a commit
that referenced
this pull request
Feb 12, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
santigimeno
added a commit
that referenced
this pull request
Feb 12, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
|
Landed in 65430e7...ed8c4f9 |
santigimeno
added a commit
that referenced
this pull request
Feb 12, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
santigimeno
added a commit
that referenced
this pull request
Feb 12, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
santigimeno
added a commit
that referenced
this pull request
Feb 14, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
santigimeno
added a commit
that referenced
this pull request
Feb 14, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
santigimeno
added a commit
that referenced
this pull request
Feb 14, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #268
santigimeno
added a commit
that referenced
this pull request
Feb 14, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #268
santigimeno
added a commit
that referenced
this pull request
Feb 14, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #269
santigimeno
added a commit
that referenced
this pull request
Feb 14, 2025
PR-URL: #266 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> PR-URL: #269
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While also adding tracing channels to track http2 client and server streams lifetime.