Skip to content

Get logs endpoint & system_stats additions#4664

Closed
pythongosssss wants to merge 4 commits intoComfy-Org:masterfrom
pythongosssss:get-logs-api
Closed

Get logs endpoint & system_stats additions#4664
pythongosssss wants to merge 4 commits intoComfy-Org:masterfrom
pythongosssss:get-logs-api

Conversation

@pythongosssss
Copy link
Copy Markdown
Member

Some additions for better error reporting

Adds new /internal/logs endpoint for getting the last 300 log entries
Updates /system_stats to include comfyui_version (if in a git repo), pytorch_version and argv for the launch args.

Using pygit2 as that is included with the Windows releases, falling back to calling git manually.

Replaces output streams with LogInterceptor which I've tested with ComfyUI Managers output hooks, not sure if other nodes do anything with these streams that may cause/have issues.

Copy link
Copy Markdown
Contributor

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

LGTM with minor nits!

Comment thread server.py Outdated
@huchenlei huchenlei added the Good PR This PR looks good to go, it needs comfy's final review. label Aug 28, 2024
Comment thread main.py Outdated
# Intercept logs for error reporting
import sys
from app.log_interceptor import LogInterceptor
sys.stdout = LogInterceptor(sys.stdout)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All the logging in core comfy uses the logging python module so is this really the best way of doing this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All the logging in core comfy uses the logging python module so is this really the best way of doing this?

We also want to intercept all stdout from custom nodes which we don't have control over. Custom nodes can just call print for console message. Currently ComfyUI-Manager is doing the same stdout object hijack to save output log file.

@huchenlei
Copy link
Copy Markdown
Contributor

From claude:

from logging.handlers import MemoryHandler
from collections import deque

def setup_memory_logger(logger_name='root', level=logging.INFO, capacity=300):
    # Get the root logger
    logger = logging.getLogger(logger_name)
    logger.setLevel(level)

    # Create a memory handler with a deque as its buffer
    memory_handler = MemoryHandler(capacity, flushLevel=logging.ERROR)
    memory_handler.buffer = deque(maxlen=capacity)

    # Create a formatter
    formatter = logging.Formatter('%(asctime)s - %(name)s - %(levelname)s - %(message)s')
    memory_handler.setFormatter(formatter)

    # Add the memory handler to the logger
    logger.addHandler(memory_handler)

    def get_log_contents():
        return '\n'.join(memory_handler.buffer)

    return logger, get_log_contents

# Usage example
if __name__ == "__main__":
    logger, get_logs = setup_memory_logger()

    # Log some messages
    for i in range(350):
        logger.info(f"Log message {i}")

    # Simulate an error
    try:
        1 / 0
    except ZeroDivisionError:
        logger.exception("Caught an exception")

    # Get the last 300 log messages
    log_contents = get_logs()
    print("Last 300 log messages:")
    print(log_contents)

So there is a MemoryHandler that can be used to cache log messages. It's less hacky than the stdout/stderr hijack.

@huchenlei
Copy link
Copy Markdown
Contributor

Offline discussion final decision: go with logging handler and see if the log satisfied our need in error reports.

@yoland68
Copy link
Copy Markdown
Member

Downside of this is that it doesn't capture millions of custom nodes that does print statement, but we can first start in this way and see whether it will end up missing context for user issue reporting
https://sourcegraph.com/search?q=repo:ComfyUI+%22++print%28%22&patternType=keyword&sm=0

@pythongosssss
Copy link
Copy Markdown
Member Author

Updated per feedback

Copy link
Copy Markdown
Contributor

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

LGTM!

@huchenlei
Copy link
Copy Markdown
Contributor

@comfyanonymous Gentle ping on the review

@robinjhuang
Copy link
Copy Markdown
Member

Nice to see another /internal route :)

Comment thread app/logger.py

# Setup default global logger
logger = logging.getLogger()
logger.setLevel(logging.INFO)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This breaks: --verbose

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Resolved
PR moved to #4690

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

Labels

Good PR This PR looks good to go, it needs comfy's final review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants