-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add Laravel Queue Integration with Span Links #2026
Conversation
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've just reviewed the tags you used based on the documentation for messaging system in confluence. Could you check your entries? I'd expect a bit more Tag::MQ_*
to be used honestly, but maybe I'm wrong.
Can you also then fill the confluence page with the laravel specifics? It's not mandatory as won't be reused by others but it's a good habit to take I believe
src/Integrations/Integrations/LaravelQueue/LaravelQueueIntegration.php
Outdated
Show resolved
Hide resolved
src/Integrations/Integrations/LaravelQueue/LaravelQueueIntegration.php
Outdated
Show resolved
Hide resolved
{ | ||
$metadata = [ | ||
'messaging.laravel.attempts' => $job->attempts(), | ||
'messaging.laravel.connection' => $job->getConnectionName() ?? config('queue.default'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHould we use messaging.url
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The connection name will be something like 'database', 'sync', or 'redis' (non-exhaustive, these are defined in the config/queue.php
file). It doesn't really have the format we would expect from a url in my opinion. However, if you feel like this makes sense, then that's ok for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Though I'd say it doesn't have the format of a connection either. Underlying system would seem more appropriate but 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this like this? 'mysql:host=mysql_integration;dbname=test'
In those cases my concerns are more about leaking sensitive info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Laravel connections are defined using an associative array (as can be seen in this sample configuration) with specific configuration keys, rather than a single string containing all parameters. Therefore, the value of messaging.laravel.connection
would be one of these configuration keys, referencing to one of the defined queue connection types. Hence, it shouldn't leak any sensitive information.
Note: A 'connection' in the context of laravel queue refers to the method by which the queue system communicates with the underlying storage mechanism used to store and retrieve queued jobs.
src/Integrations/Integrations/LaravelQueue/LaravelQueueIntegration.php
Outdated
Show resolved
Hide resolved
'messaging.laravel.max_tries' => $job->maxTries(), | ||
'messaging.laravel.timeout' => $job->timeout(), | ||
'messaging.laravel.name' => $job->resolveName(), | ||
Tag::MQ_SYSTEM => 'laravel', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this supposed to be like kafka
, rabbitmq
, rocketmq
, activemq
, AmazonSQS
, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Laravel queues aren't necessarily dispatched via these mechanisms. It's a separate mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this, I was also planning to update the confluence page to include the newly introduced span attributes; messaging.system
would be one of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple nits, but looks fine for merge then.
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Description
This PR comprises three features:
The Laravel Queue integration traces the following methods:
Illuminate\Queue\Worker::process
Illuminate\Queue\Jobs\Job::fire
Illuminate\Queue\Jobs\Job::resolve
Illuminate\Bus\Batch::add
Illuminate\Queue\Queue::enqueueUsing
Illuminate\Contracts\Queue\Queue::push
Illuminate\Contracts\Queue\Queue::later
Things to note:
Illuminate\Queue\Queue::enqueueUsing
doesn't provide much information. It just provides some context onto 'why are all these events happening'. However, I don't mind removing it.Job
, nor a string (e.g., redis), information about the connection and the queue will be tried to be retrieved. In the event that it wasn't possible to retrieve it in from the object or the parameters, the configuration defaults (config/queue.php
) will be used.post-autoload-dump
phase. Considering the sample apps are in production mode, the migrations are forced and the whole database is basically dropped and rebuilt from the migrations. This was needed to ran the jobs-related migrations.PhpApache.php
.Readiness checklist
Reviewer checklist