Azure / go-amqp

AMQP 1.0 client library for Go.
https://github.com/Azure/go-amqp
MIT License
104 stars 56 forks source link

feat: 138: Enable logging to be pluggable #327

Open zalgonoise opened 3 months ago

zalgonoise commented 3 months ago

Hi folks :D

This PR adds support for configurable logging in go-amqp using log/slog.

Note: log/slog was added on Go 1.21. This proposal involves a Go version bump from 1.18 -> 1.21

The logger is kept in a global state, within the internal/debug package, as before. However, it is initialized with a no-op implementation of a slog.Handler. Doing so, allows sane defaults (disabled logging) while allowing the caller to still register their own slog.Handler if they desire.

This is done via a RegisterLogger in internal/debug which is then made public in a top-level debug.go file. We're able to discuss the approach to this change, if you'd maybe prefer to ditch internal/debug all together, moving that logic into top-level debug.go.

The signature of Log is changed to allow consuming contexts, defining level with slog.Level values, just like Assert which should work like before, but the caller is also able to pass arguments (like slog.Attr), and making Assertf no longer necessary. Minimal tests added for both of these functions.

The new log levels respect the verbosity as defined before, but we can also discuss any changes you'd like to see in that regard.

As for a client or consumer of this library, they are able to enable and disable logging by registering a slog.Handler using amqp.RegisterLogger. This is in line of strategies seen in libraries like opentelemetry-go. Provided that the global-state objects are initialized with sane defaults, I don't perceive any particular issue with this approach, but curious on your input on this.

Calling amqp.RegisterLogger could happen from the main.go file for the client application that uses go-amqp, if the app sends and / or receives messages using this library; or by a client library that wraps go-amqp


A race condition on *frames.PerformFlow was found after publishing this PR, since this frame is overwritten in a *Session.mux call. With this implementation, passing frames as slog.Attr attributes, it causes it to be read and written to at the same time in certain occasions. This may not have been noticeable before due to the (complete) no-op implementation of the debug logging function.

Added a (private) *sync.Mutex to *frames.PerformFlow, alongside a frames.NewPerformFlow constructor and *frames.PerformFlow.Lock and *frames.PerformFlow.Unlock methods

Closes #138

zalgonoise commented 3 months ago

@microsoft-github-policy-service agree

zalgonoise commented 3 months ago

currently in Draft to fix the data races raised when reading the (pointer) elements of *frames.PerformFlow (and possibly other frames), when their String method is called when parsing a slog.Attr for any type, or when calling the frames' String method directly when available.

I will address it, then release the PR from draft once again :D

zalgonoise commented 3 months ago

addressed the issue with the race condition on *frames.PerformFlow; documented in the PRs description