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

Emitter: Try an event logger-style interface #11

Open
abhinav opened this issue Oct 19, 2022 · 2 comments
Open

Emitter: Try an event logger-style interface #11

abhinav opened this issue Oct 19, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@abhinav
Copy link
Collaborator

abhinav commented Oct 19, 2022

Instead of these several scoped objects,
we should maybe consider something like fxevent.Logger
where we have a single interface that accepts event objects,
and we have several kinds of event objects.
I'm not certain that this is desirable, but it's worth exploring.

@abhinav abhinav added the enhancement New feature or request label Oct 19, 2022
@abhinav
Copy link
Collaborator Author

abhinav commented Oct 21, 2022

Quick update on this:
I've verified that it's possible to minimize the emitter interface to:

type EventLogger interface {
	LogEvent(context.Context, Event)
}

type Event interface{ event() }

With the following events,

type FlowStopEvent struct {
	Name    string
	Runtime time.Duration
	Err     error // if any
}

type ParallelStopEvent struct {
	Name    string
	Runtime time.Duration
	Err     error // if any
}

type TaskStopEvent struct {
	Name       string
	Runtime    time.Duration // zero if skipped
	ParentType string
	ParentName string
	Err        error // if any
	PanicValue any   // if any
	Recovered  bool  // recovered from panic/err
	Skipped    bool
}

type SchedulerStateEvent struct {
	DirectiveType string
	DirectiveName string
	State         scheduler.State
}

And still implement the metrics and log emitters we need with similar efficiency.
(There may be some room for more cleanup in the events.)

So I'm going to opt to delete the current emitter implementations (#2) for now,
and if we have time, we can replace the interface before initial release.
Otherwise, we can still do it immediately after.

abhinav added a commit that referenced this issue Oct 21, 2022
This deletes the Zap and Tally emitter implementations from cff to
reduce the dependency footprint.
We'll keep copies of these for use internally.

This also warns users away from using WithEmitter because the API is
expected to change significantly with #11.

Resolves #2
abhinav added a commit that referenced this issue Oct 21, 2022
This deletes the Zap and Tally emitter implementations from cff to
reduce the dependency footprint.
We'll keep copies of these for use internally.

This also warns users away from using WithEmitter because the API is
expected to change significantly with #11.

Resolves #2
abhinav added a commit that referenced this issue Oct 21, 2022
This deletes the Zap and Tally emitter implementations from cff to
reduce the dependency footprint.
We'll keep copies of these for use internally.

This also warns users away from using WithEmitter because the API is
expected to change significantly with #11.

Resolves #2
Depends on #20
@gcmurphy
Copy link

Is this still being worked on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants