Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler #85104

Closed
mphelt opened this issue Apr 20, 2023 · 3 comments · Fixed by #85143
Closed

Missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler #85104

mphelt opened this issue Apr 20, 2023 · 3 comments · Fixed by #85143
Labels
area-Extensions-HttpClientFactory bug help wanted [up-for-grabs] Good issue for external contributors
Milestone
8.0.0

Comments

@mphelt
Copy link
Contributor

mphelt commented Apr 20, 2023

Background and motivation

Both LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler have implementations of:
protected async override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request!!, CancellationToken cancellationToken)
but they are missing implementations of:
protected internal override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
It would be great to see these implementations introduced, as otherwise custom implementation of:

  • IHttpMessageHandlerBuilderFilter,
  • HttpHeadersLogValue,
  • LoggingHttpMessageHandler
  • LoggingScopeHttpMessageHandler

is needed.
I know that synchronous usage is not expected most of the times, but we've spent quite some time figuring out why we can't turn on those logs in our legacy app migrated to .NET 6 when we tried to figure out how it works.

Risks

No response
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 20, 2023
@ghost
Copy link

ghost commented Apr 20, 2023

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

Issue Details

Background and motivation

Both LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler have implementations of:
protected async override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request!!, CancellationToken cancellationToken)
but they are missing implementations of:
protected internal override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
It would be great to see these implementations introduced, as otherwise custom implementation of:

  • IHttpMessageHandlerBuilderFilter,
  • HttpHeadersLogValue,
  • LoggingHttpMessageHandler
  • LoggingScopeHttpMessageHandler

is needed.
I know that synchronous usage is not expected most of the times, but we've spent quite some time figuring out why we can't turn on those logs in our legacy app migrated to .NET 6 when we tried to figure out how it works.

Risks

No response

Author: mphelt
Assignees: -
Labels:

untriaged, area-Extensions-HttpClientFactory
Milestone: -

@MihaZupan MihaZupan added the bug label Apr 20, 2023
@MihaZupan
Copy link
Member

Providing implementations for sync paths makes sense. In this case, it doesn't only affect performance, but also the functionality of such handlers.
@mphelt would you be interested in contributing the change?

@MihaZupan MihaZupan added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Apr 20, 2023
@MihaZupan MihaZupan added this to the Future milestone Apr 20, 2023
mphelt added a commit to mphelt/dotnet_runtime that referenced this issue Apr 21, 2023
@mphelt
add missing overrides in LoggingHttpMessageHandler and LoggingScopeHt…
7532cc5 
…tpMessageHandler (dotnet#85104)
@mphelt mphelt mentioned this issue Apr 21, 2023
Merged
@mphelt
Copy link
Contributor Author

mphelt commented Apr 21, 2023 •
edited
Loading

@MihaZupan Sure, please find #85143.
I didn't create copy of all tests that use SendAsync, but only the basic ones to check if it works - I hope that's enough.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 25, 2023
CarnaViire pushed a commit that referenced this issue May 4, 2023
@mphelt
Missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMe…
4b104fb 
…ssageHandler (#85143)

* add missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler (#85104)

* Update LoggingUriOutputTests.cs

* Update LoggingHttpMessageHandler.cs

* Update LoggingScopeHttpMessageHandler.cs

* Update LoggingScopeHttpMessageHandler.cs

* Update LoggingUriOutputTests.cs

* Update LoggingScopeHttpMessageHandler.cs

* Update LoggingUriOutputTests.cs

* Update LoggingUriOutputTests.cs

* Update LoggingUriOutputTests.cs

* Update LoggingUriOutputTests.cs

* Update LoggingHttpMessageHandler.cs

* Update LoggingScopeHttpMessageHandler.cs

* Update LoggingHttpMessageHandler.cs

* Update LoggingScopeHttpMessageHandler.cs

* Update LoggingHttpMessageHandler.cs

* Update LoggingHttpMessageHandler.cs

* Update LoggingScopeHttpMessageHandler.cs

* Code style update

* back to private methods

* merge with dotnet/runtime (#7)
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 4, 2023
@karelz karelz modified the milestones: Future, 8.0.0 May 27, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-Extensions-HttpClientFactory bug help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Milestone
8.0.0
3 participants
@karelz @MihaZupan @mphelt