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

fxevent.Logger: Silence until there's an error #1167

Open
abhinav opened this issue Feb 23, 2024 · 2 comments
Open

fxevent.Logger: Silence until there's an error #1167

abhinav opened this issue Feb 23, 2024 · 2 comments

Comments

@abhinav
Copy link
Collaborator

abhinav commented Feb 23, 2024

Is your feature request related to a problem? Please describe.
The Fx event log is useful in debugging startup failures when there's a failure.
However, in the success case, it's largely noise.

Describe the solution you'd like
A variant of fxevent.Logger that prints nothing if the service successfully starts up, but prints the full event log if there were any failures in the process.

This would probably take the form of a wrapper around fxevent.Logger that buffers messages in-memory, and flushes them if any of the individual error-able events fails, or if the entire start up fails.

I haven't thought about what the API for this would look like.

Describe alternatives you've considered

  • Live with it and ignore the hundreds of lines of meaningless logs
  • Disable the fxevent logger and lose the ability to debug

Is this a breaking change?
No

@jmmills
Copy link

jmmills commented Feb 6, 2025

@abhinav I've implemented a rudimentary version of this internally. I could raise PR with a variation of it, but a few notes and questions:

  • It seems reasonable to add a function to reduce event types to a given error if present, something like func Err(event Event) error; this might also work for a severity property should that ever become a thing
  • Would the expectation that be that a buffered error logger be thread-safe? A simple slice append of []Event wouldn't cut it if you have multiple *fx.App instances in different go-routines share the same instance of fxevent.Logger

@abhinav
Copy link
Collaborator Author

abhinav commented Feb 10, 2025

Hi again @jmmills!

It seems reasonable to add a function to reduce event types to a given error if present, something like func Err(event Event) error; this might also work for a severity property should that ever become a thing

Clarify, please? Ability to convert a non-error event to an error, or to customize the error of an error event into something else before it's logged? For the latter, I think it might make sense to delegate that to an external wrapper of the logger.
e.g. you'd be expected to implement your own fxevent.Logger that wraps the buffered fxevent.Logger, transforming events before they're fed into the buffered one, which then does buffering, etc. as needed.

Would the expectation that be that a buffered error logger be thread-safe?

I don't think so. Fx assumes single-threaded exclusive execution; the Fx container owns the resources.
A single fx.App instance instantiates its fxevent.Logger on demand using the constructor given to fx.WithLogger.
We don't usually expect multiple running fx.Apps, and in fact the APIs make it difficult.
It would take some additional gymnastics for someone to share an fxevent.Logger between two concurrently running apps, but it's not impossible.
In such an off-kilter use case, I think it would be acceptable to say that the caller is responsible for synchronizing the fxevent.Logger, as a single fx.App does not mess with it in a concurrent fashion.

It would be straightforward for someone to implement a thread-safe wrapper in such a case, so I don't think we'd need to bend for an unusual use case that much.

typye syncLogger struct {
  mu sync.Mutex
  inner fxevent.Logger
}

func (sl *syncLogger) LogEvent(ev fxevent.Event) {
  sl.mu.Lock()
  defer sl.mu.Unlock()

  sl.inner.LogEvent(ev)
}

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

No branches or pull requests

3 participants