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

feat: 138: Enable logging to be pluggable #327

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

zalgonoise
Copy link

@zalgonoise zalgonoise commented May 24, 2024

  • The purpose of this PR is explained in this or a referenced issue.
  • Tests are included and/or updated for code changes.
  • Updates to CHANGELOG.md are included.
  • MIT license headers are included in each file.

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
Copy link
Author

@microsoft-github-policy-service agree

@zalgonoise zalgonoise marked this pull request as draft May 24, 2024 23:43
…sing Go 1.20 in go.mod; bump golangci-lint to latest version
@zalgonoise
Copy link
Author

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

…nditions on concurrent reads / writes to its pointer elements; added Lock and Unlock methods; protecting String method call with mutex
…uctor frames.NewPerformFlow; lock existing frames when writing to them
@zalgonoise zalgonoise marked this pull request as ready for review May 29, 2024 23:11
@zalgonoise
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable logging to be pluggable
1 participant