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

[Feature Request]: Support for custom loggers in runtime_logging #2581

Closed
Tracked by #7
ekzhu opened this issue May 3, 2024 · 10 comments
Closed
Tracked by #7

[Feature Request]: Support for custom loggers in runtime_logging #2581

ekzhu opened this issue May 3, 2024 · 10 comments
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed logging related to logging issue

Comments

@ekzhu
Copy link
Collaborator

ekzhu commented May 3, 2024

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:

def start(logger_type: str = "sqlite", config: Optional[Dict[str, Any]] = None) -> str:

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

@bboynton97
Copy link
Collaborator

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

@shippy
Copy link
Contributor

shippy commented May 7, 2024

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.)

@ekzhu
Copy link
Collaborator Author

ekzhu commented May 7, 2024

@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

@shippy
Copy link
Contributor

shippy commented May 8, 2024

@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?

@krishnashed
Copy link
Contributor

@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 ?

@ekzhu
Copy link
Collaborator Author

ekzhu commented May 9, 2024

@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.

@shippy
Copy link
Contributor

shippy commented May 10, 2024

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?

@krishnashed
Copy link
Contributor

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

@Hk669
Copy link
Contributor

Hk669 commented May 17, 2024

@ekzhu we can close this issue right?

@Hk669
Copy link
Contributor

Hk669 commented May 17, 2024

@ekzhu we can close this issue right?

the custom logger is addressed in the merged PR #2596

@ekzhu ekzhu closed this as completed May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed logging related to logging issue
Projects
None yet
Development

No branches or pull requests

5 participants