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 in log statement #829

Merged
merged 5 commits into from Aug 12, 2018

Conversation

Projects
None yet
4 participants
@gjeee
Contributor

gjeee commented Jun 22, 2018

log statement is not printing the error message

Error in log statement
Log line did not print the message correctly
added semicolon
forgot semi-colon
@GalRogozinski

This comment has been minimized.

Show comment
Hide comment
@GalRogozinski

GalRogozinski Jun 27, 2018

Contributor

Hmm since we are catching a non-typed exception it may be helpful to see the stack trace...

@gjeee, do you have lots of ugly stack traces in your node? What motivated you to fix that?

Contributor

GalRogozinski commented Jun 27, 2018

Hmm since we are catching a non-typed exception it may be helpful to see the stack trace...

@gjeee, do you have lots of ugly stack traces in your node? What motivated you to fix that?

@gjeee

This comment has been minimized.

Show comment
Hide comment
@gjeee

gjeee Jun 27, 2018

Contributor

well my motivation was as follows:

when checking out logs i grepped only lines with "error" in it. since the printed stacktrace was printed on a newline it did not came up in my grep. so the only lines i got, were something like:

API Exception:
API Exception:
API Exception:

so this does not contain useful info. therefore imo the two options below are more suitable:

log.error("API Exception: " + e.getLocalizedMessage());

or even better:

log.error("API Exception: {}", e.getLocalizedMessage(), e);

what i caught so far mostly were InvalidAlgorithmParameterException(REFERENCE_TRANSACTION_TOO_OLD);

Contributor

gjeee commented Jun 27, 2018

well my motivation was as follows:

when checking out logs i grepped only lines with "error" in it. since the printed stacktrace was printed on a newline it did not came up in my grep. so the only lines i got, were something like:

API Exception:
API Exception:
API Exception:

so this does not contain useful info. therefore imo the two options below are more suitable:

log.error("API Exception: " + e.getLocalizedMessage());

or even better:

log.error("API Exception: {}", e.getLocalizedMessage(), e);

what i caught so far mostly were InvalidAlgorithmParameterException(REFERENCE_TRANSACTION_TOO_OLD);

@GalRogozinski

This comment has been minimized.

Show comment
Hide comment
@GalRogozinski

GalRogozinski Jun 28, 2018

Contributor

ok, I understand better now.

When we catch a general Exception we assume that

  1. It is probably unexpected
  2. We want the stack trace for easy debugging.

Since most of the time you are catching the InvalidAlgorithmParameterException: You can write the following code:
Where we run getTransactionsToApprove add before we catch the RuntimeException the following

catch(InvalidAlgorithmParameterException e) {
             log.error("API Exception: " + e.getLocalizedMessage());
             return ExceptionResponse.create(e.getLocalizedMessage());

And where you did the change write

catch(Exception e) {
            log.error("API Exception: " + e.getLocalizedMessage(), e);
            return ExceptionResponse.create(e.getLocalizedMessage());

Note a few things:

  1. log.error("API Exception: {}", e.getLocalizedMessage(), e); doesn't work. It annoys me that they don't have this signature available.
  2. log.error("API Exception: " + e.getLocalizedMessage(), e); will print the message twice which is a little ugly but you convinced me that the merits of grep justify it.
  3. Exception in IRI should be general but unfortunately it also catches RocksDb Exceptions. See #749.
  4. I actually prefer e.getMessage() just because I don't ever think we will ever use localized messages. e.getLocalizedMessage() is just misleading and creates longer code. However other parts of the code contain that string so it is ok for now and definitely shouldn't be changed in this PR.
Contributor

GalRogozinski commented Jun 28, 2018

ok, I understand better now.

When we catch a general Exception we assume that

  1. It is probably unexpected
  2. We want the stack trace for easy debugging.

Since most of the time you are catching the InvalidAlgorithmParameterException: You can write the following code:
Where we run getTransactionsToApprove add before we catch the RuntimeException the following

catch(InvalidAlgorithmParameterException e) {
             log.error("API Exception: " + e.getLocalizedMessage());
             return ExceptionResponse.create(e.getLocalizedMessage());

And where you did the change write

catch(Exception e) {
            log.error("API Exception: " + e.getLocalizedMessage(), e);
            return ExceptionResponse.create(e.getLocalizedMessage());

Note a few things:

  1. log.error("API Exception: {}", e.getLocalizedMessage(), e); doesn't work. It annoys me that they don't have this signature available.
  2. log.error("API Exception: " + e.getLocalizedMessage(), e); will print the message twice which is a little ugly but you convinced me that the merits of grep justify it.
  3. Exception in IRI should be general but unfortunately it also catches RocksDb Exceptions. See #749.
  4. I actually prefer e.getMessage() just because I don't ever think we will ever use localized messages. e.getLocalizedMessage() is just misleading and creates longer code. However other parts of the code contain that string so it is ok for now and definitely shouldn't be changed in this PR.

gjeee added some commits Jul 24, 2018

Error in log statement
* incorporated feedback GalRogozinski
* the new signature of log.info is possible since SLF4J 1.6.0. see also https://www.slf4j.org/faq.html#paramException
Error in log statement
forgot import
@gjeee

This comment has been minimized.

Show comment
Hide comment
@gjeee

gjeee Jul 24, 2018

Contributor

ok @GalRogozinski i incorporated all your feedback :-)
regarding point 1) the new signature of log.info is possible since SLF4J 1.6.0. see also https://www.slf4j.org/faq.html#paramException

Contributor

gjeee commented Jul 24, 2018

ok @GalRogozinski i incorporated all your feedback :-)
regarding point 1) the new signature of log.info is possible since SLF4J 1.6.0. see also https://www.slf4j.org/faq.html#paramException

@GalRogozinski GalRogozinski merged commit bf8293c into iotaledger:dev Aug 12, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment