HttpClient logging based on new .NET 8 runtime APIs #4224

xakep139 · 2023-07-28T15:41:43Z

Microsoft Reviewers: Open in CodeFlow

xakep139 · 2023-07-28T15:44:34Z

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/IHttpClientLogEnricher.cs

+    /// <param name="exception">An optional <see cref="Exception"/> that was thrown within the outgoing HTTP request processing.</param>
+    // TODO: adding a new parameter to the interface method is a breaking change. Pay additional attention to this during review.
+    // Alternatively, we can decide to add another overload instead.
+    void Enrich(IEnrichmentPropertyBag enrichmentBag, HttpRequestMessage request, HttpResponseMessage? response = null, Exception? exception = null);


We need to discuss this one

Is it breaking if it's defaulted to null?

No, the breaking change here is that I added Exception? exception - which means that existing users of this API would need to add the new parameter into their implementation of the interface

xakep139 · 2023-07-28T15:45:15Z

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/HttpClientLogger.cs

+            }
+
+            // Recommendation is to swallow the exception (logger shouldn't throw), so we don't re-throw here:
+            return null;


As @CarnaViire pointed, a logger shouldn't re-throw (but it will change our current behavior)

xakep139 · 2023-07-28T15:47:05Z

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/HttpClientLogger.cs

+
+    public object? LogRequestStart(HttpRequestMessage request)
+    {
+        throw new NotSupportedException();


As of now the best we can offer for sync overloads - is to call their async counterparts with .GetAwaiter().GetResult(), that's because there's no sync API of reading request/response message body

xakep139 · 2023-07-28T15:48:30Z

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/HttpClientLogger.cs

+            }
+
+            propertyBag = LogMethodHelper.GetHelper();
+            FillLogRecord(logRecord, propertyBag, in elapsed, request, response, exception);


We don't use TimeProvider here since elapsed is provided out-of-the-box, but we can consider ignoring it and using the old logic

xakep139 · 2023-07-28T15:49:26Z

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/Log.cs

+        var httpMethod = request[startIndex].Value;
+        var httpHost = request[startIndex + 1].Value;
+        var httpPath = request[startIndex + 2].Value;
+        return FormattableString.Invariant($"{httpMethod} {httpHost}/{httpPath}");


This is to align with current behavior in internal library

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/Log.cs

xakep139 · 2023-07-28T15:51:04Z

test/Libraries/Microsoft.Extensions.Http.Telemetry.Tests/Logging/Internal/TestLoggingHandler.cs

+
+    private static DelegatingHandler CreateInternalBclLoggingHandler(IHttpClientLogger logger, HttpMessageHandler? innerHandler)
+    {
+        var handlerType = typeof(IHttpClientFactory).Assembly.GetType("Microsoft.Extensions.Http.Logging.HttpClientLoggerHandler");


This was taken from Natalia's PoC branch main...CarnaViire:extensions:poc-hcf-custom-logging

Are you sure you need this? The intent on my side was just to show that our HttpClientLoggerHandler passes all the tests you had for your logging handler. Like, "you can trust our handler" 😄

So I believe the set of tests that were verifying the handler itself can be removed from dotnet/extensions repo (I haven't looked at the PR yet, I don't know how exactly you've transformed the test cases).

Or if you do need a handler for some other tests, create it "normal way" via IHttpMessageHandlerFactory.

...ibraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/LogRecordPooledObjectPolicy.cs

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/Log.cs

RussKie · 2023-07-30T21:59:27Z

MC

...braries/Microsoft.Extensions.Http.Telemetry/Latency/Internal/HttpClientLatencyLogEnricher.cs

dariusclay · 2023-07-31T07:07:25Z

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/IHttpClientLogEnricher.cs

+    /// <param name="exception">An optional <see cref="Exception"/> that was thrown within the outgoing HTTP request processing.</param>
+    // TODO: adding a new parameter to the interface method is a breaking change. Pay additional attention to this during review.
+    // Alternatively, we can decide to add another overload instead.
+    void Enrich(IEnrichmentPropertyBag enrichmentBag, HttpRequestMessage request, HttpResponseMessage? response = null, Exception? exception = null);


Is it breaking if it's defaulted to null?

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/IHttpClientLogEnricher.cs

fix XML docs

use XUnit consistently in a test class

RussKie · 2023-08-16T03:56:35Z

Please re-target to release/8.0, if this is necessary for .NET 8.

xakep139 · 2023-08-16T08:55:46Z

Please re-target to release/8.0, if this is necessary for .NET 8.

I would rather merge it to main first, and then cherry-pick the commit into release/8.0.
Right now, if I target release/8.0, the PR brings a few unrelated changes from main, e.g. .github/fabricbot.json, etc.

remove orphaned file

RussKie · 2023-08-17T02:56:16Z

MC

xakep139 · 2023-08-17T10:35:46Z

MC

Resolved

* use new .NET 8 runtime APIs in HttpClient logging * Leverage KeyedServices feature in HttpClient logging (#4253) Co-authored-by: Nikita Balabaev <[email protected]>

xakep139 added 10 commits July 18, 2023 18:51

fixes + tests

4614a5a

try to use new logging APIs

8e30724

minor fix

8dc3a32

update dependencies

abf5d88

Merge branch 'main' into xakep139/outgoing-http-logging

8a5ae6a

implement async logging part

46b3c52

simplify extensions

e1eb602

fixes + further dev

e9fee43

further dev

2026789

fixes + update tests

57be31d

xakep139 added the area-telemetry label Jul 28, 2023

xakep139 requested a review from geeknoid July 28, 2023 15:41

xakep139 self-assigned this Jul 28, 2023

xakep139 commented Jul 28, 2023

View reviewed changes

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/Log.cs Outdated Show resolved Hide resolved

xakep139 commented Jul 28, 2023

View reviewed changes

xakep139 requested review from CarnaViire and JamesNK July 28, 2023 15:53

geeknoid reviewed Jul 28, 2023

View reviewed changes

...ibraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/LogRecordPooledObjectPolicy.cs Outdated Show resolved Hide resolved

geeknoid reviewed Jul 28, 2023

View reviewed changes

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/Internal/Log.cs Show resolved Hide resolved

dariusclay reviewed Jul 31, 2023

View reviewed changes

xakep139 and others added 4 commits July 31, 2023 16:49

Merge branch 'main' into xakep139/outgoing-http-logging

bff6a42

CR

7ae1508

Update src/LegacySupport/TaskWaitAsync/TaskExtensions.cs

82a3f0c

fix a warning

f223f8b

fixes after merge

687509c

xakep139 marked this pull request as ready for review August 14, 2023 11:25

Merge branch 'main' into xakep139/outgoing-http-logging

cf5daef

xakep139 commented Aug 14, 2023

View reviewed changes

src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/IHttpClientLogEnricher.cs Show resolved Hide resolved

xakep139 added 3 commits August 14, 2023 16:20

resolve a TODO item;

af7dbf1

fix XML docs

fix tests;

5f69923

use XUnit consistently in a test class

Merge branch 'main' into xakep139/outgoing-http-logging

ac0d2eb

xakep139 changed the base branch from main to release/8.0 August 16, 2023 08:59

xakep139 changed the base branch from release/8.0 to main August 16, 2023 09:03

xakep139 added 2 commits August 16, 2023 11:04

Merge branch 'main' into xakep139/outgoing-http-logging

67605df

increase code coverage;

c05d8e1

remove orphaned file

RussKie added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Aug 17, 2023

Merge branch 'main' into xakep139/outgoing-http-logging

9b1ea6e

ghost removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Aug 17, 2023

xakep139 added 2 commits August 21, 2023 10:42

Merge branch 'main' into xakep139/outgoing-http-logging

045a6ca

resolve the last TODO item

7203050

xakep139 enabled auto-merge (squash) August 21, 2023 08:45

xakep139 merged commit 16b51f2 into main Aug 21, 2023
6 checks passed

xakep139 deleted the xakep139/outgoing-http-logging branch August 21, 2023 09:11

ghost added this to the 9.0-preview1 milestone Aug 21, 2023

xakep139 mentioned this pull request Aug 21, 2023

[release/8.0] HttpClient logging based on new .NET 8 runtime APIs (#4224) #4319

Merged

xakep139 mentioned this pull request Aug 22, 2023

Remove default values in IHttpClientLogEnricher #4323

Merged

RussKie removed the area-telemetry label Aug 29, 2023

ghost locked as resolved and limited conversation to collaborators Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpClient logging based on new .NET 8 runtime APIs #4224

HttpClient logging based on new .NET 8 runtime APIs #4224

xakep139 commented Jul 28, 2023 •

edited by ghost

Loading

xakep139 Jul 28, 2023

dariusclay Jul 31, 2023

xakep139 Jul 31, 2023

xakep139 Jul 28, 2023

xakep139 Jul 28, 2023

xakep139 Jul 28, 2023

xakep139 Jul 28, 2023

xakep139 Jul 28, 2023

CarnaViire Jul 28, 2023 •

edited

Loading

RussKie commented Jul 30, 2023

dariusclay Jul 31, 2023

RussKie commented Aug 16, 2023

xakep139 commented Aug 16, 2023 •

edited

Loading

RussKie commented Aug 17, 2023

xakep139 commented Aug 17, 2023

HttpClient logging based on new .NET 8 runtime APIs #4224

HttpClient logging based on new .NET 8 runtime APIs #4224

Conversation

xakep139 commented Jul 28, 2023 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarnaViire Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

RussKie commented Jul 30, 2023

Choose a reason for hiding this comment

RussKie commented Aug 16, 2023

xakep139 commented Aug 16, 2023 • edited Loading

RussKie commented Aug 17, 2023

xakep139 commented Aug 17, 2023

xakep139 commented Jul 28, 2023 •

edited by ghost

Loading

CarnaViire Jul 28, 2023 •

edited

Loading

xakep139 commented Aug 16, 2023 •

edited

Loading