Refactor removal of built-in HttpClient logging handler

[JamesNK]

Implementation of what is discussed at dotnet/runtime#85840 (comment)

It's a bit more complicated than I described because filters run after HttpMessageHandlerBuilderActions configuration. I worked around that by adding a filter that removes the handlers added by the logging filter.

AddDefaultHttpClientLogging_DisablesNetScope passes after this change.

cc @geeknoid @dpk83 @CarnaViire @noahfalk

Microsoft Reviewers: Open in CodeFlow

[JamesNK]

@geeknoid Is dotnet/runtime#85840 still required after this change? It allows you to disable built-in HTTP client logging without removing IHttpMessageHandlerBuilderFilter configuration set by the user.

[noahfalk]

Does this disable the logging for this specific builder, or for all builders? I think this fixes the removing other non-logging filters, but it still has an issue that if R9 registers logging for one HttpClientBuilder it disables logging for all of them.

[JamesNK]

It disables the built-in logging for all builders.

The feedback at dotnet/runtime#85840 was that it should be disabled everywhere.

[noahfalk]

I think dotnet/runtime#85840 has only talked about the global case when R9 has both global and per-clientbuilder scenarios. IMO if someone called AddHttpClientLogging(this IHttpClientBuilder builder, ...) to use the new logging for one client builder they are not expecting built-in logging for all other unrelated client builders to be disabled. Does that sound right or I am missing something?

[noahfalk]

@geeknoid @dpk83

[JamesNK]

Ok, I see what you mean. I added support for selective disable.

[noahfalk]

Is part of the plan to change the naming or behavior of dotnet/extension logging? Perhaps that is for another PR/prototype but I think we should either:

• Change the naming to make it clear these methods are opting into an alternative strategy. For example UseMinimalHttpLogging()
• Change the behavior of these methods so that prior to configuring any LoggingOptions they match the existing HttpClient logging output. For example there might be a LoggingOptions.LoggingMode property with enum options that are either Classic or Light. By default it would be Classic and have all the existing behavior, and Light has the new behavior that logs much less. Hopefully all (or most) of the other options can compose regardless whether Classic or Light has been chosen.
• Do something like what ASP.NET Core logging is doing where the built-in logging gains the configurability and dotnet/extensions is layering specific scoped features on top like redaction and enrichment. The APIs would then be named based on the specific capability being added.

As-is the naming and user experience of these methods feels confusing to me. There is already default http logging behavior that comes with AddHttpClient() so if this API has any effect then it must be adding non-default behavior despite the "default" in the name.

[JamesNK]

I think the Default in the extension method name AddDefaultHttpClientLogging refers to how this change is applied to all HTTP clients by default.

[noahfalk]

I agree that was probably the intent, but I think it would be pretty easy for our developers not to understand that. Also even if they do interpret 'default' in that way it still leaves the question "I already have Http logging behavior before I called this API, so what behavior is this API adding?"

If our hands are tied and we have to have this API named as-is we can certainly try to mitigate with docs, but it doesn't feel nice to me.

[CarnaViire]

I agree with @noahfalk, the current naming here is super confusing. And if e.g. later we in BCL will go forward with API similar to what I proposed in dotnet/runtime#85840, then we'd have both AddDefaultHttpClientLogging and AddHttpClientDefaults().ConfigureLogging(o => o.AddDefaultProviders()) and it would be really hard to tell the difference and which to use (and too many defaults 😄). (And BCL either won't be able to adopt AddHttpClientLogging name for our API or we'd have conflicting names). I also agree that "default" associates more strongly with the logging we have in BCL (behavior that comes with AddHttpClient()).

Do something like what ASP.NET Core logging is doing where the built-in logging gains the configurability and dotnet/extensions is layering specific scoped features on top like redaction and enrichment. The APIs would then be named based on the specific capability being added.

This would be the ideal solution IMO. I wonder if we'd have enough time and capacity to do that (in this release at least), but I would love to go with that way.

But regardless, I think it should be either clear from the API name that that's the alternative logging, or it all should be as one big layered logging solution.

[CarnaViire]

In general, the workaround hacky but clever, so that should always work for current implementation, thanks @JamesNK!

I need to consider how it would stack with AddHttpClientDefaults() API I wanted to add, because it returns the same builder type, meaning your logging APIs could also be called; I need to figure out how to make both work in an expected way

[geeknoid]

OK gang, if this works and you're comfortable to implement this logic in this package, then I'm fine with it and we can close dotnet/runtime#85840.

But I'd like to confirm with Deepak that this is sufficient @dpk83?

[dpk83]

OK gang, if this works and you're comfortable to implement this logic in this package, then I'm fine with it and we can close dotnet/runtime#85840.
But I'd like to confirm with Deepak that this is sufficient @dpk83?

Yeah, I am good with it.