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.

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

Use the logging generator in LoggingChatClient / LoggingEmbeddingGenerator #5508

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Oct 10, 2024

Microsoft Reviewers: Open in CodeFlow

Comment on lines +172 to +173
[LoggerMessage(LogLevel.Trace, "{MethodName} invoked: {ChatMessages}. Options: {ChatOptions}. Metadata: {ChatClientMetadata}.")]
private partial void LogInvokedSensitive(string methodName, string chatMessages, string chatOptions, string chatClientMetadata);
Copy link
Member

Choose a reason for hiding this comment

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

As it's already checked if the LogLevel is enabled, there's no need for the SGen to generate that check too. So set

Suggested change
[LoggerMessage(LogLevel.Trace, "{MethodName} invoked: {ChatMessages}. Options: {ChatOptions}. Metadata: {ChatClientMetadata}.")]
private partial void LogInvokedSensitive(string methodName, string chatMessages, string chatOptions, string chatClientMetadata);
[LoggerMessage(LogLevel.Trace, "{MethodName} invoked: {ChatMessages}. Options: {ChatOptions}. Metadata: {ChatClientMetadata}.", SkipEnabledCheck = true)]
private partial void LogInvokedSensitive(string methodName, string chatMessages, string chatOptions, string chatClientMetadata);

in order to avoid the redundant check be emitted.

Same on other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't do so because these are only Debug/Trace events, and in the case where such events are enabled, the extra interface call will be dwarfed by all the JSON serialization and other overheads. And while I could just enable it anyway, not all of the call sites are currently guarded, which makes it complicated for someone maintaining the code to know which call sites need guarding and which don't. Which then leads to consistently putting SkipEnabledCheck on all of the log methods and consistently guarding all call sites, but that in turn makes the code more expensive. Seemed best to just keep it simple and pay for the extra enabled check when there's already a ton of overhead / when debugging.

@stephentoub stephentoub merged commit 85e70b0 into dotnet:main Oct 11, 2024
6 checks passed
@stephentoub stephentoub deleted the improvelogging branch October 11, 2024 12:56
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants