autogen icon indicating copy to clipboard operation
autogen copied to clipboard

[Feature Request]: Support for custom loggers in runtime_logging

Open ekzhu opened this issue 1 year ago • 8 comments

Is your feature request related to a problem? Please describe.

In runtime_logging, we currently use a factory method to create a logger in the start function when we start logging. This requires us to support all logger backends in our library. We want to support using custom logger for better integration experience.

Describe the solution you'd like

In runtime_logging:

https://github.com/microsoft/autogen/blob/093c1a26e399b1f2d4614f41bc3e561b2fa74e21/autogen/runtime_logging.py#L23

Adding a new parameter logger: BaseLogger to allow for custom logger. This logger must implement the BaseLogger protocol.

We can additionally add a logger that simply writes to a file.

Additional context

Related issues and PRs: #2423 #2516

ekzhu avatar May 03 '24 18:05 ekzhu

One option would be to look at Langchain's callback pattern and implement something similar.

bboynton97 avatar May 06 '24 19:05 bboynton97

Would it maybe make sense to replace logger_type with logger_cls: Type[BaseLogger] and then try to directly call its constructor? Like so:

class LoggerFactory:
    @staticmethod
    def get_logger(logger_cls: Type[BaseLogger] = SqliteLogger, config: Optional[Dict[str, Any]] = None) -> BaseLogger:
        if config is None:
            config = {}

        try:
            return logger_cls(config)
        except:
            raise ValueError(f"[logger_factory] Could not initialize logger with logger class: {logger_cls}")

(I don't like the amount of problems that can arise with the logger initialization - might make sense to check that it correctly subclasses BaseLogger first as the type hint indicates it should.)

shippy avatar May 07 '24 06:05 shippy

@shippy is there a usage case that might require the framework to maintain the loggers? I am thinking of us moving away from maintaining the logger as it has caused problem in parallel execution #2617

ekzhu avatar May 07 '24 23:05 ekzhu

@ekzhu Until AutoGen makes it big enough for the big players to instrument it automatically, I think there might be a case to be made for providing a set of bindings to e.g. Logfire or OpenLLMetry, or at least provide an example for how to do that. The OTel nature of those loggers should hopefully obviate the parallel execution effects?

shippy avatar May 08 '24 08:05 shippy

@ekzhu Until AutoGen makes it big enough for the big players to instrument it automatically, I think there might be a case to be made for providing a set of bindings to e.g. Logfire or OpenLLMetry, or at least provide an example for how to do that. The OTel nature of those loggers should hopefully obviate the parallel execution effects?

What do you think about this @ekzhu should I also look into this ?

krishnashed avatar May 08 '24 09:05 krishnashed

@shippy for "maintaining the logger" I meant the AutoGen framework in the runtime creating and maintaining the logger object in the framework stack.

Providing bindings for telemetry systems seems like something we can do with a custom logger? @krishnashed would love for you to take a look into this. #2596 adds support for custom logger.

ekzhu avatar May 09 '24 17:05 ekzhu

Thanks for the link to the current implementation PR! I think basically what's missing right now is a way to directly expose and wrap the OpenAI/other instrumentation, the way logfire.instrument_openai or langsmith.wrappers.wrap_openai would like to. The logger would have to do this at agent creation time, wouldn't it?

shippy avatar May 10 '24 05:05 shippy

Thanks @ekzhu for the PR link, @shippy ill take a look at logfire/langsmith, can you please share any links for better understanding. and let me get back to you

krishnashed avatar May 10 '24 06:05 krishnashed

@ekzhu we can close this issue right?

Hk669 avatar May 17 '24 10:05 Hk669

@ekzhu we can close this issue right?

the custom logger is addressed in the merged PR #2596

Hk669 avatar May 17 '24 10:05 Hk669