contribsys / faktory_worker_go

Faktory workers for Go
Mozilla Public License 2.0
242 stars 43 forks source link

Add middleware support #6

Closed koesie10 closed 6 years ago

koesie10 commented 6 years ago

I added chained middleware support on manager and job type level, so middleware can also be added on an individual job. I also added context.JobType() to allow retrieving the job type because it can be useful in middleware.

Example usage:

mgr.Use(func(perform worker.Perform) worker.Perform {
    return func(ctx worker.Context, args ...interface{}) error {
        log.Printf("Starting work on job %s of type %s\n", ctx.Jid(), ctx.JobType())
        err := perform(ctx, args...)
        log.Printf("Finished work on job %s with error %v\n", ctx.Jid(), err)
        return err
    }
})

When applied in the test package, it will transform this output:

2018/02/02 12:42:41 Working on job p5d9qprOQK6k_2dU
2018/02/02 12:42:41 Context &{context.Background p5d9qprOQK6k_2dU SomeJob}
2018/02/02 12:42:41 Args [1 2 hello]

Into this output:

2018/02/02 12:42:41 Starting work on job p5d9qprOQK6k_2dU of type SomeJob
2018/02/02 12:42:41 Working on job p5d9qprOQK6k_2dU
2018/02/02 12:42:41 Context &{context.Background p5d9qprOQK6k_2dU SomeJob}
2018/02/02 12:42:41 Args [1 2 hello]
2018/02/02 12:42:41 Finished work on job j-XwEx2tFS93I6Op with error <nil>

Example usage with Logrus and only logging errors:

entry := logrus.WithField("system", "faktory")

mgr.Use(func(perform worker.Perform) worker.Perform {
    return func(ctx worker.Context, args ...interface{}) error {
        err := perform(ctx, args...)
        entry := entry.WithFields(logrus.Fields{
            "jid":     ctx.Jid(),
            "jobtype": ctx.JobType(),
        })
        if err != nil {
            entry.WithError(err).Errorf("Failed to execute job")
        }
        return err
    }
})
mperham commented 6 years ago

I don't like the idea of job-specific middleware. Global middleware is easy to understand and grok. Job-specific middleware is better designed into the job itself.

mperham commented 6 years ago

Sorry if that started overly negative. Thank you for this work, this is nice to see.

koesie10 commented 6 years ago

You are right, job-level middleware don't make too much sense. I've removed them from the commit.

mperham commented 6 years ago

Middleware usually acts on attributes in job.Custom. You will want to expose the job's custom attributes to the middleware (but hide the custom data from the actual perform method, for separation of concerns).

koesie10 commented 6 years ago

I've made some changes that allow middleware to access the complete Job struct. The way this middleware is implemented requires every handler function to be the same, so such a handler is constructed in Register that wraps the Perform function. I'm not completely happy with this solution, but right now I can't think of a better way to make sure the perform function doesn't have access to complete job information, while allowing middleware the access.

mperham commented 6 years ago

This looks like a great start. Further polish is always welcome.

mperham commented 6 years ago

Thank you!