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

Enable logging to be pluggable #138

Open
richardpark-msft opened this issue Mar 7, 2022 · 3 comments · May be fixed by #327
Open

Enable logging to be pluggable #138

richardpark-msft opened this issue Mar 7, 2022 · 3 comments · May be fixed by #327
Assignees
Labels
feature-request This issue requires a new behavior in the product in order be resolved.

Comments

@richardpark-msft
Copy link
Member

Today, the only method for enabling go-amqp logging is to recompile your binary with a debug tag. However, this output is just goes to stdout/stderr and isn't capturable by the caller.

This means that libraries like azservicebus, which have their own pluggable logging systems (via azcore/log), aren't able to redirect those log messages to the output source the customer chose. It also means we can't automatically activate the logging, at runtime since it's compile-time only.

As part of this it's a good chance to look at our logging and see which parts might be optional - errors that do bubble up to the customer with enough tracing information don't need to be logged. However, FLOW frames, which "flow" in the background, or when channels being blocked when writing/reading (indicating potential performance issues) will be, as these are typically not visible.

@RickWinter RickWinter added the feature-request This issue requires a new behavior in the product in order be resolved. label May 2, 2022
@patrick246
Copy link

Hi, I'd like to take a stab at this, as we're currently missing the ability to turn on logging selectively in production, while debugging some AMQP-related problems. Ideally, we could have different log levels, to increase verbosity up to actual procotol logs if needed.

Do you have any plans for how this pluggable logging should look like? Should this library use azcore/log, should it just specify an interface that you can adapt various logging frameworks into, or should we use log/slog from the standard library and adapt other logging libraries through that?

@jhendrixMSFT
Copy link
Member

Richard and I have discussed this but don't have a firm design in mind yet. What we do know is that whatever we come up with, it needs to be 100% pay-for-play.

Can we get some more info on the types of issues you're seeing?

@zalgonoise zalgonoise linked a pull request May 24, 2024 that will close this issue
4 tasks
@zalgonoise
Copy link

zalgonoise commented May 24, 2024

Hi, I've opened #327 with a proposal for this issue :D it's using log/slog, allowing the logger's slog.Handler to be configured by a client / consumer of this library. If you'd like we can discuss that approach, in case you have not decided on a particular strategy for debug logging.

to summarize, the biggest gain is the ease-of-access to this information without having to worry about the debug tag. As a plus, we can leverage the change to migrate logging to use log/slog

Note: there are caveats to this proposal including the Go version for log/slog as well as synchronization in *frames.PerformFlow. We're able to discuss those caveats to discuss on better solutions for them if by any means they go against to what you're planning in having within Azure/go-amqp :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants