Skip to content

Improve tracing of HTTP/2 PINGs - #57747

Merged
antonfirsov merged 2 commits into
dotnet:mainfrom
antonfirsov:http/http2-ping-logging-2
Aug 23, 2021
Merged

Improve tracing of HTTP/2 PINGs#57747
antonfirsov merged 2 commits into
dotnet:mainfrom
antonfirsov:http/http2-ping-logging-2

Conversation

@antonfirsov

@antonfirsovantonfirsov commented Aug 19, 2021

Copy link
Copy Markdown
Contributor

#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.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

#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 can meet the bar).

Contributes to #57617.

Author:antonfirsov
Assignees:-
Labels:

area-System.Net.Http

Milestone:-

@antonfirsov
antonfirsov requested review from a team and geoffkizerAugust 19, 2021 17:32
@karelz

Copy link
Copy Markdown
Member

@antonfirsov for 6.0 backport, can we somehow easily check that when the logging is enabled, nothing bad happens and/or that we get what we need? (e.g. with induced error)

@karelz

Copy link
Copy Markdown
Member

CI failures: System.Threading.Tasks.Tests - known problem #57331, re-running tests

@karelzkarelz added this to the 7.0.0 milestone Aug 20, 2021
@antonfirsov

Copy link
Copy Markdown
ContributorAuthor

I pushed a test improvement to make sure both failure cases (ThrowProtocolError() calls) are covered.

Did a local run of all flow control tests having an EventListener attached, and everything worked as expected.

@antonfirsov

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov

Copy link
Copy Markdown
ContributorAuthor

Outerloop test failures are unrelated and likely external outages: #46439#41381

@antonfirsov
antonfirsov merged commit 84ca939 into dotnet:mainAug 23, 2021
@antonfirsov

Copy link
Copy Markdown
ContributorAuthor

/backport to release/6.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1158715829

@karelz

Copy link
Copy Markdown
Member

/backport to release/6.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1161483669

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@antonfirsov@karelz@ManickaP@MihaZupan