Skip to content

Conversation

jjhou666
Copy link

@jjhou666 jjhou666 commented Sep 5, 2025

User description

-Use Rougamo-based logging attribute to captures method exception details, parameters, and BotSharp-specific context.
-This attribute can be used across all BotSharp plugins for consistent function logging.


PR Type

Enhancement


Description

  • Add Rougamo-based exception logging attribute for BotSharp functions

  • Captures method exceptions with context details and parameters

  • Provides consistent logging across all BotSharp plugins

  • Extracts conversation, agent, and function metadata automatically


Diagram Walkthrough

flowchart LR
  A["Method Exception"] --> B["FnExceptionLogAttribute"]
  B --> C["Extract Logger"]
  B --> D["Extract Function Context"]
  C --> E["Log Error Details"]
  D --> E
  E --> F["Structured Log Output"]
Loading

File Walkthrough

Relevant files
Enhancement
FnExceptionLogAttribute.cs
Add comprehensive function exception logging attribute     

src/Infrastructure/BotSharp.Core/Infrastructures/LogAttributes/FnExceptionLogAttribute.cs

  • Implements Rougamo-based exception logging attribute
  • Extracts logger and service provider from target objects
  • Captures BotSharp-specific context (conversation ID, agent, function)
  • Formats structured error logs with method parameters
+187/-0 

@jjhou666 jjhou666 changed the title Add Function log exception attribute Add function exception log attribute Sep 5, 2025
@jjhou666 jjhou666 marked this pull request as ready for review September 8, 2025 15:29
Copy link

qodo-merge-pro bot commented Sep 8, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
Serializing and logging method arguments (including RoleDialogModel content and arbitrary objects via JsonSerializer) can leak PII or secrets into logs. Add redaction, opt-in fields, maximum length truncation, and type-based exclusions.

⚡ Recommended focus areas for review

Missing Usings

The code references types like ILogger, ILoggerFactory, RoleDialogModel, and JsonSerializer without visible using directives. Ensure required namespaces are imported to avoid build errors.

using BotSharp.Abstraction.Functions;
using Microsoft.Extensions.Logging.Abstractions;
using Rougamo;
using Rougamo.Context;
using Rougamo.Metadatas;
using System.Reflection;
Logger Resolution Robustness

Reflection-based logger/service provider retrieval may be brittle and costly. Consider caching results or supporting DI via constructor to avoid reflection on every exception path.

private ILogger? GetLogger(MethodContext context)
{
    try
    {
        var target = context.Target;
        var targetType = target?.GetType();

        if (targetType == null) return NullLogger.Instance;

        var loggerField = targetType?.GetFields(System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)
            .FirstOrDefault(f => f.FieldType == typeof(ILogger) ||
                                (f.FieldType.IsGenericType &&
                                 f.FieldType.GetGenericTypeDefinition() == typeof(ILogger<>)));

        if (loggerField != null)
        {
            return loggerField.GetValue(target) as ILogger;
        }

        var serviceProviderField = targetType?.GetFields(System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)
            .FirstOrDefault(f => f.FieldType == typeof(IServiceProvider));

        if (serviceProviderField != null)
        {
            var serviceProvider = serviceProviderField.GetValue(target) as IServiceProvider;
            var loggerFactory = serviceProvider?.GetService<ILoggerFactory>();
            return loggerFactory?.CreateLogger(targetType) ??
                   NullLogger.Instance;
        }

        return NullLogger.Instance;
    }
    catch
    {
        return NullLogger.Instance;
    }
}
Sensitive Args Logging

Argument serialization may log sensitive data (e.g., RoleDialogModel content). Provide redaction/whitelisting or size limits to prevent leaking PII and overly large logs.

private void LogMethodError(ILogger? logger, MethodContext context, FunctionContext functionContext, Exception? ex)
{
    if (logger == null || !logger.IsEnabled(LogLevel.Error)) return;

    var argumentsSummary = string.Empty;
    if (_logArguments && context?.Arguments?.Length > 0)
    {
        argumentsSummary = string.Join(", ", context.Arguments.Select((arg, i) =>
            $"arg{i}: {GetArgumentSummary(arg)}"));
    }

    logger.LogError(ex,
            "[FUNCTION_ERROR] {MethodName} | ConvId: {ConversationId} | Channel: {Channel} | AgentId: {AgentId} | Function: {FunctionName} | MsgId: {MessageId} | Exception: {ExceptionType} | Message: {ExceptionMessage}{ArgumentsInfo}",
            functionContext.MethodFullName,
            functionContext.ConversationId,
            functionContext.Channel,
            functionContext.CurrentAgentId,
            functionContext.FunctionName,
            functionContext.MessageId,
            ex?.GetType().Name ?? "Unknown",
            ex?.Message ?? "No message",
            !string.IsNullOrEmpty(argumentsSummary) ? $" | Args: [{argumentsSummary}]" : "");
}

private string GetArgumentSummary(object arg)
{
    if (arg == null) return "null";

    try
    {
        if (arg is string str)
        {
            return $"\"{str}\"";
        }

        if (arg is RoleDialogModel message)
        {
            return $"RoleDialogModel(Role: {message.Role}, Content: {message.Content})";
        }

        if (arg.GetType().IsPrimitive || arg is decimal || arg is DateTime)
        {
            return arg.ToString();
        }

        var json = JsonSerializer.Serialize(arg);
        return json;
    }
    catch
    {
        return $"{arg.GetType().Name}(...)";
    }
}

Copy link

qodo-merge-pro bot commented Sep 8, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Make argument logging safe-by-default

Logging full method arguments (including RoleDialogModel.Content) by default
risks leaking PII/secrets and can serialize large/complex objects, causing
performance issues. Make argument logging opt-in with redaction/truncation and
type filters (e.g., skip streams/byte arrays), and allow whitelisting of safe
fields instead of serializing entire objects. Provide configuration to cap sizes
and mask known sensitive keys (token, password, content), and only log user
message content when explicitly enabled.

Examples:

src/Infrastructure/BotSharp.Core/Infrastructures/LogAttributes/FnExceptionLogAttribute.cs [20-24]
    public FnExceptionLogAttribute(
        bool logArguments = true)
    {
        _logArguments = logArguments;
    }
src/Infrastructure/BotSharp.Core/Infrastructures/LogAttributes/FnExceptionLogAttribute.cs [144-172]
    private string GetArgumentSummary(object arg)
    {
        if (arg == null) return "null";

        try
        {
            if (arg is string str)
            {
                return $"\"{str}\"";
            }

 ... (clipped 19 lines)

Solution Walkthrough:

Before:

public class FnExceptionLogAttribute : AsyncMoAttribute
{
    private readonly bool _logArguments;

    public FnExceptionLogAttribute(bool logArguments = true) // Logging is ON by default
    {
        _logArguments = logArguments;
    }

    private string GetArgumentSummary(object arg)
    {
        if (arg is RoleDialogModel message)
        {
            // Logs full content, which can be PII
            return $"RoleDialogModel(Role: {message.Role}, Content: {message.Content})";
        }
        // Serializes any other object to JSON, risking secret leaks
        return JsonSerializer.Serialize(arg);
    }
}

After:

public class FnExceptionLogAttribute : AsyncMoAttribute
{
    private readonly bool _logArguments;

    public FnExceptionLogAttribute(bool logArguments = false) // Logging is OFF by default
    {
        _logArguments = logArguments;
    }

    private string GetArgumentSummary(object arg)
    {
        if (arg is Stream) return "[omitted stream]";
        if (arg is RoleDialogModel message)
        {
            // Content is redacted by default
            return $"RoleDialogModel(Role: {message.Role}, Content: [redacted])";
        }
        // Use a safe serializer that redacts sensitive keys and truncates long values
        return SafeJsonSerializer.Serialize(arg);
    }
}
Suggestion importance[1-10]: 9

__

Why: This suggestion addresses a critical security and performance flaw where logging is enabled by default, potentially exposing PII and sensitive data from arguments like RoleDialogModel.Content.

High
Security
Configure JSON serialization safely

Using JsonSerializer without proper configuration can cause circular reference
exceptions or expose sensitive data. Add serialization options to prevent these
issues and limit output size for logging.

src/Infrastructure/BotSharp.Core/Infrastructures/LogAttributes/FnExceptionLogAttribute.cs [165-166]

-var json = JsonSerializer.Serialize(arg);
-return json;
+var options = new JsonSerializerOptions
+{
+    ReferenceHandler = ReferenceHandler.IgnoreCycles,
+    MaxDepth = 3,
+    WriteIndented = false
+};
+var json = JsonSerializer.Serialize(arg, options);
+return json.Length > 500 ? json.Substring(0, 500) + "..." : json;
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion prevents potential JsonException due to circular references by correctly configuring the serializer, significantly improving the robustness of the argument logging feature.

Medium
General
Check properties for logger instances

The reflection search should also check properties in addition to fields, as
many classes expose loggers through properties rather than fields. This ensures
broader compatibility with different logging patterns.

src/Infrastructure/BotSharp.Core/Infrastructures/LogAttributes/FnExceptionLogAttribute.cs [44-47]

 var loggerField = targetType?.GetFields(System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)
     .FirstOrDefault(f => f.FieldType == typeof(ILogger) ||
                         (f.FieldType.IsGenericType &&
                          f.FieldType.GetGenericTypeDefinition() == typeof(ILogger<>)));
 
+if (loggerField == null)
+{
+    var loggerProperty = targetType?.GetProperties(System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.Public)
+        .FirstOrDefault(p => p.FieldType == typeof(ILogger) ||
+                            (p.PropertyType.IsGenericType &&
+                             p.PropertyType.GetGenericTypeDefinition() == typeof(ILogger<>)));
+    
+    if (loggerProperty != null)
+    {
+        return loggerProperty.GetValue(target) as ILogger;
+    }
+}
+
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the logger discovery mechanism is incomplete by only searching for fields; extending the search to properties makes the attribute more robust and widely applicable.

Low
  • Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant