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.

Sign up for GitHub

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 new configuration options for the job logging #147

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add new configuration options for the job logging #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e443ac
Added new configuration options for the job logging
vesper8 Jan 3, 2024
d4fa8ef
Add new configuration options for the job logging
vesper8 Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions config/bref.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,20 @@

'request_context' => false,

/*
|--------------------------------------------------------------------------
| Log Level
|--------------------------------------------------------------------------
|
| Here you can configure which job events will trigger a log message.
|
*/

'logger_jobs' => [
'processing' => true,
'processed' => true,
'failed' => true,
'failed_details' => true,
],
Copy link
Member

Choose a reason for hiding this comment

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

I'd go with one setting for all, after looking at the logs I'm not sure it's worth having different options.

How about jobs_logs true/false?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more a global log_level that controls all the output, or maybe individual keys for the different components? Like injecting secrets.

Copy link
Member

Choose a reason for hiding this comment

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

log_level sounds good (super clear), but as a user I'd be wondering what the levels are, and what they do. That's added choice on the user, which adds complexity.

Considering the logs in question, what would be the levels? On and off? That's kinda like a boolean?

i.e. I'm not against it, but I have trouble seeing what that means in practice.

Copy link
Member

Choose a reason for hiding this comment

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

I was initially thinking the eight RFC 5424 levels (debug, info, notice, warning, error, critical, alert, emergency).

But maybe something like this would be easier:

  'log' => [
    'init' => true,
    'queue' => true,
  ],

But of course ALWAYS log any kind or failure or warning.

Copy link
Member

Choose a reason for hiding this comment

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

I also just noticed that the messages in this PR are actually going through the LogManager so @vesper8 can theoretically already filter them out. See #148.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the whole point of log levels that apps have a standardized way or logging messages and the administrator can control which logs he cares about?

Yes.

Our only problem is the init stuff from #143, because the LogManager hasn't been booted, yet. But maybe there is a way around this @georgeboot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah completely missed that 🤭

Yeah we should be able to buffer the bootup messages somehow, until we have a hard crash or booted logger.

Copy link
Member

Choose a reason for hiding this comment

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

Do you feel like tackling this?

Copy link
Author

Choose a reason for hiding this comment

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

Any update on this... can we maybe accept my PR for now if you don't want to tackle the more complex approach. I'm getting thousands of "Processed Job" logs on my dev where Bref is not even doing anything and it's driving me mad

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote to make a simple and single 'log jobs' setting, default true.

It would be great if Laravel would allow you to set the severity threshold per package instead of just global. But afaiaa that's not a thing.


];
66 changes: 37 additions & 29 deletions src/BrefServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,35 +74,43 @@ public function boot(Dispatcher $dispatcher, LogManager $logManager, FailedJobPr
], 'bref-config');
}

$dispatcher->listen(
fn (JobProcessing $event) => $logManager->info(
"Processing job {$event->job->getJobId()}",
['name' => $event->job->resolveName()]
)
);

$dispatcher->listen(
fn (JobProcessed $event) => $logManager->info(
"Processed job {$event->job->getJobId()}",
['name' => $event->job->resolveName()]
)
);

$dispatcher->listen(
fn (JobExceptionOccurred $event) => $logManager->info(
"Job failed {$event->job->getJobId()}",
['name' => $event->job->resolveName()]
)
);

$dispatcher->listen(
fn (JobFailed $event) => $queueFailer->log(
$event->connectionName,
$event->job->getQueue(),
$event->job->getRawBody(),
$event->exception
)
);
if(config('bref.logger_jobs.processing', true)) {
$dispatcher->listen(
fn(JobProcessing $event) => $logManager->info(
"Processing job {$event->job->getJobId()}",
['name' => $event->job->resolveName()]
)
);
}

if(config('bref.logger_jobs.processed', true)) {
$dispatcher->listen(
fn(JobProcessed $event) => $logManager->info(
"Processed job {$event->job->getJobId()}",
['name' => $event->job->resolveName()]
)
);
}

if(config('bref.logger_jobs.failed', true)) {
$dispatcher->listen(
fn(JobExceptionOccurred $event) => $logManager->info(
"Job failed {$event->job->getJobId()}",
['name' => $event->job->resolveName()]
)
);
}

if(config('bref.logger_jobs.failed_details', true)) {
$dispatcher->listen(
fn(JobFailed $event) => $queueFailer->log(
$event->connectionName,
$event->job->getQueue(),
$event->job->getRawBody(),
$event->exception
)
);
}
}

/**
Expand Down