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

Proposal: add middleware hook just before apply(mod, fun, args) in Task started by worker #438

Closed
hubertlepicki opened this issue Jan 15, 2021 · 2 comments

Comments

@hubertlepicki
Copy link

After this PR is released #436 (comment), Exq will start Task (it currently just spawns). In either case, I am missing a piece of functionality that I am happy to add myself, provided it's welcome addition (I don't want to maintain Exq fork myself for ever).

The thing I need is a middleware hook that will execute just before apply(m, f, a) is called here:
https://github.com/akira/exq/blob/master/lib/exq/worker/server.ex#L178

I don't need it, but doing so, we may also want to add a middleware hook _after just to be consistent/flexible.

So, the code in question could look like:

  def dispatch_work(worker_module, job, metadata) do
    # trap exit so that link can still track dispatch without crashing
    Process.flag(:trap_exit, true)
    worker = self()

    {:ok, pid} =
      Task.start_link(fn ->
        :ok = Metadata.associate(metadata, self(), job)
        before_apply(...)
        result = apply(worker_module, :perform, job.args)
        after_apply(...)
        GenServer.cast(worker, {:done, result})
      end)

    Process.monitor(pid)
  end

The main reason for this is that I want to provide Exq orchestration for AppSignal, and since their recent release of version 2.0, they have logger backend that reports exceptions and crashes as they happen. Since, this can be done better from Exq middleware's after_failed_work(pipeline) hook, one would ideally disable that additional exception reporting for the job itself, to avoid creating double errors in AppSignal.

Currently it's difficult to do so, as there's no way to tell AppSignal to ignore given PID, because in before_work we don't have yet future PID of the Task, and there are no middleware hooks in the Task itself.

I could update each and every "perform" function in my project, or use Decorator lib to do so automatically, so that first thing it does is to tell Appsignal to ignore current PID. But this is not always the desired case as I re-use some of the code by calling Mod.perform() functions directly outside Exq.

So, I am proposing addition of these two middleware hooks: before_apply and after_apply and I can provide implementation if maintainers here think it's useful for someone else.

If not, I will be looking for some other solution, that probably will end up being wrapping worker_module into something else but it will mess things up slightly as everything will look like one worker type.

@akira @ananthakumaran do you folks think this is useful addition?

@ananthakumaran
Copy link
Collaborator

I am not really convinced at this point because it increases the Exq API surface. I am interested in seeing more use cases. That said, we have been using the below pattern from the start, this could easily be converted into a macro to reduce the boilerplate code.

  def perform(params) do
    Utils.setup_worker_metadata(__MODULE__)

    Utils.with_max_timeout(fn ->
      do_perform(params)
    end)
  end

Another way to think about this is, does this new proposal opens up any new functionality that's not currently possible. It doesn't look like from what I understand.

@hubertlepicki
Copy link
Author

That's correct, I can do that. Alrighty.

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

No branches or pull requests

2 participants