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

Expand exception stack trace in log in debug mode #5478

Merged
merged 3 commits into from
Oct 23, 2013

Conversation

PVince81
Copy link
Contributor

Log exception stack trace in debug mode instead of just the message.

Based on the discussion in #5273

Please review @karlitschek @DeepDiver1975 @tanghus

@ghost
Copy link

ghost commented Oct 22, 2013

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1714/

$message = $ex->getMessage();
if ($ex->getCode()) {
$message .= ' [' . $message . ']';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the brackets if the code is truthy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's supposed to output $ex->getCode() in brackets.
Will fix.

@karlitschek
Copy link
Contributor

good idea 👍

@tanghus
Copy link
Contributor

tanghus commented Oct 22, 2013

And tested. This is really useful 👍

if ($ex->getCode()) {
$message .= ' [' . $message . ']';
}
\OCP\Util::writeLog('index', $message, \OCP\Util::FATAL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be explicit, maybe make it \OCP\Util::writeLog('index', 'Exception: ' . $message, \OCP\Util::FATAL);

@PVince81
Copy link
Contributor Author

@bantu @tanghus Please check my latest changes:

  • moved logException to OCP\Util
  • fixed error code outputting
  • added "Exception: " prefix to all exception-related message, including stack trace. Should make it easy to grep.

@ghost
Copy link

ghost commented Oct 23, 2013

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1725/

@tanghus
Copy link
Contributor

tanghus commented Oct 23, 2013

I'm not really sure this should be in the public namespace. If so, it should take an app argument, because otherwise it would all be logged as index.

@PVince81
Copy link
Contributor Author

Makes sense. I've added the $app parameter.

@ghost
Copy link

ghost commented Oct 23, 2013

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1736/

@tanghus
Copy link
Contributor

tanghus commented Oct 23, 2013

👍 from my side. Since this is an addition to the public API I'd like another reviewer though.

@PVince81
Copy link
Contributor Author

@bantu @DeepDiver1975 ?

@DeepDiver1975
Copy link
Member

👍

DeepDiver1975 added a commit that referenced this pull request Oct 23, 2013
Expand exception stack trace in log in debug mode
@DeepDiver1975 DeepDiver1975 merged commit 8c69a53 into master Oct 23, 2013
@DeepDiver1975 DeepDiver1975 deleted the core-logexceptionstacktrace branch October 23, 2013 14:19
@lock lock bot locked as resolved and limited conversation to collaborators Aug 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants