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

Add new configuration options for the job logging #147

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

Conversation

vesper8
Copy link

@vesper8 vesper8 commented Jan 3, 2024

This PR adds the ability to turn off the logging for jobs while keeping the defaults to the current behaviour

'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.

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.

4 participants