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

Error log improvements: Provide priority level #14995

Merged
merged 1 commit into from
Aug 9, 2019

Conversation

mfb
Copy link
Contributor

@mfb mfb commented Aug 8, 2019

Overview

CRM_Core_Error::debug_var() should have a $priority argument so that we can assign a priority level to fatal errors and other usages of CRM_Core_Error::debug_var(). This is a reattempt at a lighter-weight version of #14222.

Before

A call like CRM_Core_Error::debug_var('Fatal Error Details', ...) logs at the default severity level.

After

  • CRM_Core_Error::debug_var() has an optional $priority arg.

@civibot
Copy link

civibot bot commented Aug 8, 2019

(Standard links)

@civibot civibot bot added the master label Aug 8, 2019
@eileenmcnaughton eileenmcnaughton changed the base branch from master to 5.17 August 8, 2019 21:09
@civibot civibot bot added 5.17 and removed master labels Aug 8, 2019
@eileenmcnaughton
Copy link
Contributor

@mfb I think I merged the other b4 cutting the rc - I tried just switching this over to be with it but looks like I need you to rebase some commits out

@mfb
Copy link
Contributor Author

mfb commented Aug 8, 2019

Well... can you remove #14222 from the RC? because I think it needs more work - there is some problem re: high or possibly infinite memory usage.

@eileenmcnaughton
Copy link
Contributor

@mfb ah ok - do you want to put up a revert commit for it against the rc? Then we can re-open this including that against master

@mfb
Copy link
Contributor Author

mfb commented Aug 8, 2019

Revert PR @ #14996

@seamuslee001 seamuslee001 changed the base branch from 5.17 to master August 8, 2019 22:03
@civibot civibot bot added master and removed 5.17 labels Aug 8, 2019
@seamuslee001
Copy link
Contributor

I've switched this back to master now that the revert has happened

@mfb mfb changed the title Followup for #14222: Do not print prefix in the log. Error log improvements: Provide priority level Aug 9, 2019
@eileenmcnaughton
Copy link
Contributor

@pfigel any chance you can review this?

Copy link
Contributor

@pfigel pfigel left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix @mfb, LGTM!

  • General standards
    • (r-explain) PASS
    • (r-user) PASS: Adds a new optional argument, no user impact.
    • (r-doc) PASS
    • (r-run) PASS: Could not trigger any obvious issues, confirmed that (error) logging still works.
  • Developer standards

@@ -962,7 +964,7 @@ public static function exceptionHandler($pearError) {
* $obj
*/
public static function nullHandler($obj) {
CRM_Core_Error::debug_log_message("Ignoring exception thrown by nullHandler: {$obj->code}, {$obj->message}");
CRM_Core_Error::debug_log_message("Ignoring exception thrown by nullHandler: {$obj->code}, {$obj->message}", FALSE, '', PEAR_LOG_ERR);
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been inconclusively bike-shedding on whether this should be PEAR_LOG_DEBUG or PEAR_LOG_ERR, but I don't think it matters much, so I'm good unless anyone has a strong opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These definitely may be legitimate errors that administrators may need to take action to correct.

If there is truly debug-level info being sent to the nullhandler, then some logic to sort that out from the errors could be a nice further improvement.

@eileenmcnaughton
Copy link
Contributor

thanks @pfigel - no strong opinion on the err name -n merging

@eileenmcnaughton eileenmcnaughton merged commit 3465210 into civicrm:master Aug 9, 2019
@mfb mfb deleted the context-fix branch August 9, 2019 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants