Skip to content

Missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler - #85143

Merged
CarnaViire merged 26 commits into
dotnet:mainfrom
mphelt:main
May 4, 2023
Merged

Missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler#85143
CarnaViire merged 26 commits into
dotnet:mainfrom
mphelt:main

Conversation

@mphelt

@mpheltmphelt commented Apr 21, 2023•
edited by CarnaViire
Loading

Copy link
Copy Markdown
Contributor

add missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler

Fixes#85104

@ghostghost added area-Extensions-HttpClientFactory community-contribution Indicates that the PR has been added by a community member labels Apr 21, 2023
@ghost

ghost commented Apr 21, 2023

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

add missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler (#85104)

Author:mphelt
Assignees:-
Labels:

area-Extensions-HttpClientFactory, community-contribution

Milestone:-

@mpheltmphelt mentioned this pull request Apr 21, 2023
Closed
@mphelt

mphelt commented Apr 21, 2023

Copy link
Copy Markdown
ContributorAuthor

@dotnet-policy-service agree

This was referenced Apr 21, 2023
Closed
Closed
@mphelt
mphelt marked this pull request as draft April 21, 2023 12:03
Wraith2
Wraith2 reviewed Apr 21, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingHttpMessageHandler.cs Outdated
@CarnaViire

CarnaViire commented Apr 25, 2023

Copy link
Copy Markdown
Member

@mphelt thanks for your contribution!
You need to surround your overrides in #if NET5_0_OR_GREATER because synchronous Send was only added in .NET 5: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpmessagehandler.send
And test should have a filter as well: [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNetCore))]

@mphelt

mphelt commented Apr 25, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@CarnaViire

You need to surround your overrides in #if NET5_0_OR_GREATER

I've added them, but I didn't thought of it initially, as none of the base classes has those conditions - how are they different?

@Wraith2
I've fixed those typos in both my copy of the remarks as well as the source one.

@build-analysisbuild-analysisBot mentioned this pull request Apr 26, 2023
Closed
This was referenced Apr 26, 2023
Closed
Closed
MihaZupan
MihaZupan reviewed Apr 26, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingHttpMessageHandler.cs Outdated
Comment on lines +79 to +88
var shouldRedactHeaderValue = _options?.ShouldRedactHeaderValue ?? _shouldNotRedactHeaderValue;

// Not using a scope here because we always expect this to be at the end of the pipeline, thus there's
// not really anything to surround.
Log.RequestStart(_logger, request, shouldRedactHeaderValue);
var stopwatch = ValueStopwatch.StartNew();
var response = base.Send(request, cancellationToken);
Log.RequestEnd(_logger, response, stopwatch.GetElapsedTime(), shouldRedactHeaderValue);

return response;

@MihaZupanMihaZupanApr 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we deduplicate this logic between Send and SendAsync (same for LoggingScopeHttpMessageHandler)?

E.g. by extracting it to a helper method

privateasyncTask<HttpResponseMessage>SendAsyncCore(HttpRequestMessagerequest,boolasync,CancellationTokencancellationToken)

@mphelt
mphelt marked this pull request as ready for review April 27, 2023 11:53
@build-analysisbuild-analysisBot mentioned this pull request Apr 27, 2023
Closed
MihaZupan
MihaZupan approved these changes Apr 27, 2023
View reviewed changes

@MihaZupanMihaZupan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments regarding the code style used in this repo, otherwise looks good, thanks.

Unfortunately I had to add #if NET5_0_OR_GREATER directives inside useAsync ? ... : ... - is there a cleaner way to do that?

I think it's okay as-is.

Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingHttpMessageHandler.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingHttpMessageHandler.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingHttpMessageHandler.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingHttpMessageHandler.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingScopeHttpMessageHandler.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingScopeHttpMessageHandler.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingScopeHttpMessageHandler.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Http/src/Logging/LoggingScopeHttpMessageHandler.cs Outdated
@CarnaViire

CarnaViire commented Apr 28, 2023

Copy link
Copy Markdown
Member

I've added them, but I didn't thought of it initially, as none of the base classes has those conditions - how are they different?

It's because previously only .NET Standard 2.0 API surface was used, which includes only async send: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.delegatinghandler.sendasync?view=netstandard-2.0 -- because Microsoft.Extensions.Http is compiled for .NET Standard 2.0.
But we missed the fact that when it's used in .NET 5+ apps, the sync API can be used, but it won't be overridden.

Handlers that this PR is changing are not sealed, should I make those helper methods protected virtual?

This would be adding the helper method to the public surface, which should go through API review process: https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md
In general, I don't think it would bring too much value, as it's more of an implementation detail. Users can still derive from and substitute the logging handlers -- they would just need to override both of the Sends, which is true for deriving from DelegatingHandler as well.
As for configuring logging, there's a separate umbrella issue for that: #77312

@mphelt

mphelt commented May 4, 2023

Copy link
Copy Markdown
ContributorAuthor

@CarnaViire, thank you for explanation. Is there any way to re-run those failing checks? They seem to have failed for reasons not connected to this particular PR.

@CarnaViire

CarnaViire commented May 4, 2023

Copy link
Copy Markdown
Member

@mphelt I believe the failures were fixed in #85304, could you please update to the latest main? Thanks!

CarnaViire
CarnaViire approved these changes May 4, 2023
View reviewed changes

@CarnaViireCarnaViire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@CarnaViireCarnaViire mentioned this pull request May 4, 2023
Closed
@CarnaViire

CarnaViire commented May 4, 2023

Copy link
Copy Markdown
Member

Test failure is unrelated #85772

@CarnaViire
CarnaViire merged commit 4b104fb into dotnet:mainMay 4, 2023
@karelzkarelz added this to the 8.0.0 milestone May 27, 2023
@ghostghost locked as resolved and limited conversation to collaborators Jun 26, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@CarnaViireCarnaViireCarnaViire approved these changes
@MihaZupanMihaZupanMihaZupan approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@radicalradicalAwaiting requested review from radical
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara
@thaystgthaystgAwaiting requested review from thaystg
@kgkgAwaiting requested review from kg
@lambdageeklambdageekAwaiting requested review from lambdageek
@vargazvargazAwaiting requested review from vargaz
@BrzVladBrzVladAwaiting requested review from BrzVlad
@kotlarmiloskotlarmilosAwaiting requested review from kotlarmilos
@SamMonoRTSamMonoRTAwaiting requested review from SamMonoRT
@lateralusXlateralusXAwaiting requested review from lateralusX
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky
+1 more reviewer
@Wraith2Wraith2Wraith2 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Extensions-HttpClientFactorycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Missing overrides in LoggingHttpMessageHandler and LoggingScopeHttpMessageHandler

5 participants

@mphelt@CarnaViire@Wraith2@MihaZupan@karelz