[4.x dev] Added support for handler-url and headers per job

[aaajeetee]

See the discussion: #137

The idea is that you can have the handler url defined per job, so the job can decide where this task must be handled (e.g. low performance or high performance containers).
Also, while we're at it, added the option to have task headers per job. Might be useful to have job specific headers, which can be picked up by the handler and act accordingly.

I'm looking forward to any feedback.

[marickvantuil]

Let's say there is a staging and production environment. How can I use a separate high performance queue in production (because it's high traffic) but use the same single queue setup in staging?
Or when both environments do have a high-performance queue, they need to point to different URLs which will need a configuration and environment variable. How can we make that possible?
I feel like you'd encounter that situation quickly, which is why I toyed with the idea of putting this into the config/queue.php settings:

Code snippet

'cloudtasks' => [
    'handler'               => env('STACKKIT_CLOUD_TASKS_HANDLER', ''),
    'service_account_email' => env('STACKKIT_CLOUD_TASKS_SERVICE_EMAIL', ''),
    'signed_audience'       => env('STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE', true),
    // The default handler
    'handler'               => env('STACKKIT_CLOUD_TASKS_HANDLER'),
    // Queue specific handlers
    'queue_handlers'        => [
      'low_performance' => env('STACKKIT_CLOUD_TASKS_LOW_PERF_HANDLER', env('STACKKIT_CLOUD_TASKS_HANDLER')),
      'high_performance' => env('STACKKIT_CLOUD_TASKS_HIGH_PERF_HANDLER', env('STACKKIT_CLOUD_TASKS_HANDLER')),
    ]
],

[aaajeetee]

Good point. I understand the need and use case, but is this something this package needs to take care of, or is this logic that can be put in the taskHandlerUrl() method of the job? Because I can imagine that jobs A and B will behave differently than jobs C and D, url wise (and environment) wise.

I do like the option to have multiple handlers in the config, but personally I would not hard-code these. Other than a 'default'.

Something like this:

'cloudtasks' => [
    'handler'               => env('STACKKIT_CLOUD_TASKS_HANDLER', ''),
    'service_account_email' => env('STACKKIT_CLOUD_TASKS_SERVICE_EMAIL', ''),
    'signed_audience'       => env('STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE', true),
    'handlers'              => [
      'default' => env('STACKKIT_CLOUD_TASKS_HANDLER'),
      // add your own handler urls here
    ],
],

The package or project that implements this cloud-tasks package is free to update the config as they desire and add other handlers.

With this config setup, the taskHandlerUrl() implementation of the job can fetch the appropriate handler url from the config if needed. Or add other logic to return the right handler url for that job. That's up to the job.

The main 'problem' I got with these hardcoded handlers: not all projects have "low performance" or "high performance".
Without going into too much detail: my use case is that I have 2 different handlers: the default and one with a static outgoing IP address. This does not fit the hardcoded performance handlers.

Let me know what you think of this, or perhaps have other suggestion(s) based on this.

[marickvantuil]

Nice, I really like this. I think we can remove the Queue::connection()->setTaskHeaders(...) so there is a single unified way to add task headers. If tasks require the same set of headers, a trait can be created:

trait WithDefaultTaskHeaders
{
    public function taskHeaders(): array
    {
        return [];
    }
}

Much more clear where the task headers are defined rather than a global method somewhere in a service provider.

[aaajeetee]

Yeah I personally also want to have 1 way to add task headers, but was doubting if I should remove the setTaskHeaders() on the connection or not.

Will do so now.

[marickvantuil]

Thanks! I left some feedback. Let me know what you think.

[aaajeetee]

Thanks! I left some feedback. Let me know what you think.

Thanks for the feedback!

[Plytas]

Thanks for your work @aaajeetee!

As I understand, with this PR we would be able to add HasTaskHandlerUrl interface to our jobs and then implement taskHandlerUrl() to achieve queue based routing like this?:

public function taskHandlerUrl(): string
{
    return 'https://example.com/' . $this->queue;
}

Quite possibly extracting this to a trait we can reuse. However, I do not see the need for us to route individual jobs and forgetting to implement HasTaskHandlerUrl (human error) will lead us wondering why it doesn't work.

I think there is a simpler approach for our use case and I hope you'll consider it. For example, we could setup config like this

'cloudtasks' => [
    'handler'               => env('STACKKIT_CLOUD_TASKS_HANDLER', ''),
    'service_account_email' => env('STACKKIT_CLOUD_TASKS_SERVICE_EMAIL', ''),
    'signed_audience'       => env('STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE', true),
    // The default handler
    'handler'               => env('STACKKIT_CLOUD_TASKS_HANDLER'),
    
    // New config values
    'use_queue_based_routing' => env('STACKKIT_CLOUT_TASKS_USE_QUEUE_BASED_ROUTING', false),
    'queue_based_routing_prefix' => env('STACKKIT_CLOUT_TASKS_QUEUE_BASED_ROUTING_PREFIX', env('APP_URL')),
],

And then CloudTasksQueue::getHandler() could look like this:

public function getHandler(): string
{
    if ($this->config['use_queue_based_routing']) {
        return $this->config['queue_based_routing_prefix'] . $this->queue;
    }

    return Config::getHandler($this->config['handler']);
}

Note

I wrote this by hand and haven't tested it. Perhaps building the route should happen in Config class which could ensure that use_queue_based_routing is a valid URL and would add a trailing / if it's missing one.

[aaajeetee]

Thanks for your work @aaajeetee!

As I understand, with this PR we would be able to add HasTaskHandlerUrl interface to our jobs and then implement taskHandlerUrl() to achieve queue based routing like this?:

public function taskHandlerUrl(): string
{
    return 'https://example.com/' . $this->queue;
}

Quite possibly extracting this to a trait we can reuse. However, I do not see the need for us to route individual jobs and forgetting to implement HasTaskHandlerUrl (human error) will lead us wondering why it doesn't work.

I think there is a simpler approach for our use case and I hope you'll consider it. For example, we could setup config like this

'cloudtasks' => [
    'handler'               => env('STACKKIT_CLOUD_TASKS_HANDLER', ''),
    'service_account_email' => env('STACKKIT_CLOUD_TASKS_SERVICE_EMAIL', ''),
    'signed_audience'       => env('STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE', true),
    // The default handler
    'handler'               => env('STACKKIT_CLOUD_TASKS_HANDLER'),
    
    // New config values
    'use_queue_based_routing' => env('STACKKIT_CLOUT_TASKS_USE_QUEUE_BASED_ROUTING', false),
    'queue_based_routing_prefix' => env('STACKKIT_CLOUT_TASKS_QUEUE_BASED_ROUTING_PREFIX', env('APP_URL')),
],

And then CloudTasksQueue::getHandler() could look like this:

public function getHandler(): string
{
    if ($this->config['use_queue_based_routing']) {
        return $this->config['queue_based_routing_prefix'] . $this->queue;
    }

    return Config::getHandler($this->config['handler']);
}

Note

I wrote this by hand and haven't tested it. Perhaps building the route should happen in Config class which could ensure that use_queue_based_routing is a valid URL and would add a trailing / if it's missing one.

Thanks for your suggestion. I understand your use case and see that in your case the "url per job" would add unnessecary overhead in your application.

I'm happy to add this config as well, but that means there are 4 ways to get a handler url:

1. none (= fallback to current url)
2. "default" (= the default configured handler url)
3. per job
4. queue-based (+ prefix).

I am not sure if that is the way to go? I think there can be even more use cases, which in turn can lead to more config options.
Perhaps, if there are so many different use cases, why not allow a closure to be used in the handler url? The closure could get the current job as argument.
With this closure (and job), all above mentioned use cases can be implemented I guess. The important part is that (the right) argument(s) get passed to this closure.

I know the current version (v3) of the package supports closure as well, but it does not have any arguments. (I am currently using this closure in my project with a hacky workaround to have job-specific handler urls)

What do you think @marickvantuil and @Plytas ?

[Plytas]

You are right. Supporting all these use cases is very convenient from package consumers perspective, but might turn out to be maintenance hell. I think ultimately the decision will have to be made by Stackkit team.

Perhaps we could open the customization in a Laravel'y way. We could add an ability to add this to your own service provider:

CloudTasks::configureHandlerUsing(function ($job): string {
    //perform whatever you need to perform and return task handler url
});

[aaajeetee]

You are right. Supporting all these use cases is very convenient from package consumers perspective, but might turn out to be maintenance hell. I think ultimately the decision will have to be made by Stackkit team.

Perhaps we could open the customization in a Laravel'y way. We could add an ability to add this to your own service provider:

CloudTasks::configureHandlerUsing(function ($job): string {
    //perform whatever you need to perform and return task handler url
});

I like this the most to be honest, good suggestion.

[marickvantuil]

@Plytas @aaajeetee Really like that idea as well!

[aaajeetee]

So I've updated the code. I have added a (static) configureHandlerUrlUsing method on the CloudTasksQueue class. Let me know what you think of this.

If this is the road we want to go, do we want the same implementation for the headers (setTaskHeaders) method? Because now you can call 1 method statically and the other one "magically" via Queue::connection()->setTaskHeaders().
Also, if you have mixed queue connections in your application, I can see this going wrong ("call to undefined function setTaskHeaders on XYQueue" for example).

[marickvantuil]

If this is the road we want to go, do we want the same implementation for the headers (setTaskHeaders) method?

Agreed, same implementation would be preferred. Thanks!

[aaajeetee]

Apologies for the late reply, but I updated the code accordingly. Let me know what you think.

[marickvantuil]

Looks good to me, thank you! I hope to release 4.x soon, I'm just finishing things up and doing some more testing.