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

Job tracking #11

Closed
wants to merge 3 commits into from
Closed

Job tracking #11

wants to merge 3 commits into from

Conversation

cykirsch
Copy link
Contributor

@cykirsch cykirsch commented Feb 8, 2020

Add job tracking via middleware available in Laravel 6+ as well as via job events.

This splits framework init into its own collector so that it doesn't log duplicates between HTTP and Job.

…a job events.

This splits framework init into its own collector so that it doesn't log duplicates between HTTP and Job.

protected function registerEventListeners(): void
{
$this->app->events->listen(JobProcessing::class, function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually thinking that this should be opposite. The order of execution is JobProcessing, Middleware, JobProcessed. So as it is now the Job processing span extends a few milliseconds beyond the end of the transaction.

I did it this way to intentionally be similar to the HTTP Request, but since it's not fully accurate, I'm wondering if you'd prefer it switched--the transaction would be handled by the events and the middleware would just provide an additional span. This would also have the benefit of automatic transactions for pre-L6 installs.

@cykirsch
Copy link
Contributor Author

@arkaitzgarro Any thoughts on this one so far?

@cykirsch
Copy link
Contributor Author

FWIW I'm about done with a re-implementation doing the opposite as described above; if it works out I could do a PR for that and then you can give feedback on whichever is your preferred approach.

…laravel into job_tracking

# Conflicts:
#	src/Agent.php
#	src/Collectors/HttpRequestCollector.php
@cykirsch cykirsch mentioned this pull request Feb 11, 2020
@arkaitzgarro
Copy link
Owner

Do you have an example of how both implementations visually look like in ELastic APM?

@cykirsch
Copy link
Contributor Author

Do you have an example of how both implementations visually look like in ELastic APM?

Pretty subtle difference in the demo, but you can see right at the end where the transaction ends in comparison to the "Job processing" span.

Transaction started/stopped in the event:
Screen Shot 2020-02-11 at 11 57 33 AM

Transaction started/stopped in the middleware:
Screen Shot 2020-02-11 at 12 02 09 PM

@cykirsch
Copy link
Contributor Author

Building your own demo like the above is trivial. Sharing an artisan command below, but you can do the same thing in a controller to see the job-within-a-request.

<?php

namespace App\Console\Commands;

use App\Jobs\ApmJobDemoJob;
use Illuminate\Console\Command;
use Illuminate\Contracts\Bus\Dispatcher;

class ApmJobDemo extends Command
{
    /**
     * The name and signature of the console command.
     *
     * @var string
     */
    protected $signature = 'apm:job-demo';

    /**
     * The console command description.
     *
     * @var string
     */
    protected $description = 'Demo job tracking in Elastic APM';

    /**
     * Execute the console command.
     *
     * @param Dispatcher $dispatcher
     */
    public function handle(Dispatcher $dispatcher)
    {
        $dispatcher->dispatch(new ApmJobDemoJob());
    }
}
<?php

namespace App\Jobs;

use AG\ElasticApmLaravel\Jobs\Middleware\RecordTransaction;
use App\Models\Eloquent\User;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Queue\SerializesModels;

class ApmJobDemoJob implements ShouldQueue
{
    use SerializesModels;

    public function handle()
    {
        $user = User::first();
    }

    public function middleware()
    {
        return [
            app(RecordTransaction::class),
        ];
    }
}

@arkaitzgarro
Copy link
Owner

Closing in favour of #18

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

Successfully merging this pull request may close these issues.

None yet

2 participants