Reconsider LoggingChatClient default output

[SteveSandersonMS]

Two parts of the behavior surprised me:

• The distinction between "log sensitive data" and "don't log sensitive data" is controlled by whether the log level is debug or trace. Do we use similar rules to control sensitive data logging elsewhere? I would have expected this to be separately configured, independently of log level.
• When streaming, it emits a log message for every chunk. That seems overwhelming to me. I'd expect the default to be logging only the start/end of streaming calls, not every chunk.

Not saying these are necessarily wrong, but want to raise them for discussion.

[stephentoub]

When streaming, it emits a log message for every chunk. That seems overwhelming to me. I'd expect the default to be logging only the start/end of streaming calls, not every chunk.

You think it should instead do what OpenTelemetryChatClient does, grabbing all the chunks and then logging the result of ToChatResponse at the end?

My thinking for logging each chunk was that someone who's using LoggingChatClient for debugging, demoing, etc. would like to be able to see the chunks streaming in, which is also why they're logged at the Debug level.

I'm fine with us changing that if you think it's unnatural.

The distinction between "log sensitive data" and "don't log sensitive data" is controlled by whether the log level is debug or trace. Do we use similar rules to control sensitive data logging elsewhere? I would have expected this to be separately configured, independently of log level.

LogLevel.Trace is documented as "Logs that contain the most detailed messages. These messages may contain sensitive application data.", which seemed appropriate. It's also how I've seen ASP.NET systems do what seemed like similar things, e.g. MVC logging arguments when Trace is enabled:
https://github.com/dotnet/aspnetcore/blob/0bdd885b80b5778da850802e97112a065cc9bf2b/src/Mvc/Mvc.Core/src/Infrastructure/ControllerActionInvoker.cs#L639-L643
https://github.com/dotnet/aspnetcore/blob/0bdd885b80b5778da850802e97112a065cc9bf2b/src/Mvc/Mvc.Core/src/Infrastructure/ControllerActionInvoker.cs#L626-L635

Again, if this isn't the expected way, we can change it, but based on looking around it seemed to be how LogLevel was often utilized.

[stephentoub]

@SteveSandersonMS, do you want to change anything here? Now would be the time if yes, otherwise this can be closed.

[SteveSandersonMS]

The point about PII and trace level was settled during threat modelling review I think, so no action needed there.

As for logging every chunk independently, yes I do think it would be valuable to change that. Or if we really want both worlds, we could do something like:

• At trace level, we log every chunk
• At trace and debug levels, we log the aggregated content once it completes

Update: no that doesn't make sense. We can only log any content at trace level as it may be PII...

[stephentoub]

I put up #6425 to remove the Debug-level per-update logging. Let me know if you think more is needed.

[SteveSandersonMS]

Nice, thanks for handling that so quickly. I was about to do so but was on a call, so am glad we didn't both do it!