[release/6.0] Improve tracing of HTTP/2 PINGs #57930
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
ghost
added
the
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsBackport of #57747 to release/6.0 /cc @antonfirsov Customer ImpactTestingRisk
|
Member
|
@steveisok a build failure in tvOS it looks like. |
Member
|
Logging only, to help investigate customer reports. Approved for release/6.0 |
karelz
approved these changes
Aug 23, 2021
Member
|
@danmoseley any thoughts on RC1 consideration? Should we try it with Tactics? |
Member
|
You're making an RC1 PR, as it was approved there, but no harm in merging this one meantime. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
#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 @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.