[release/6.0-rc1] Improve tracing of HTTP/2 PINGs #57998
Merged
Conversation
This file contains hidden or 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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=80&v=4){kind=small}
ghost
added
the
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsBackport of #57747 to release/6.0-rc1 /cc @karelz @antonfirsov Customer ImpactTestingRisk
|
karelz
approved these changes
Aug 24, 2021
|
Approved by Tactics for RC1 on 8/23 via email. |
|
Unrelated runtime-staging failures, rerunning them just in case ... |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Backport of #57747 to release/6.0-rc1
#57617 can be a worrying reliability issue. In case there will be other reports, it is very important to have the ability to collect traces that can help hunting down a potential bug in HttpClient or Kestrel. Currently we are not logging received PING content at all, which prevents investigation based on customer data.
This is a minimal-risk (log only) addition to #54755, which I believe we should backport to 6.0 (including RC2, and maybe RC1 if it meets the bar).
Contributes to #57617.
/cc @karelz @antonfirsov
Customer Impact
#57617 can be a worrying reliability issue (potential 6.0 regression from #54755). Currently we are not logging received PING content at all, which blocks investigation of PING-related PROTOCOL_ERROR, based on customer data (like reported in #57617). In case we get more customer reports, it is important to have the ability to collect traces that can help investigating and fixing a potential bug caused by #54755.
Testing
The PR adds an additional test to make sure all RTT PING PROTOCOL_ERROR code paths are covered. I did a local run of all
SocketsHttpHandler_Http2FlowControl_Testtests with an EventListener attached.Risk
Minimal. This is a log-only change.