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

RFE: New SLF4J_LOST_EXCEPTION #74

Open
vorburger opened this issue Apr 20, 2018 · 3 comments
Open

RFE: New SLF4J_LOST_EXCEPTION #74

vorburger opened this issue Apr 20, 2018 · 3 comments

Comments

@vorburger
Copy link
Contributor

Proposal for a new check which flags this up as wrong:

} catch (SomeException e) {
    LOG.error("...");
}

but this is, typically, what you want people to do instead of course:

} catch (SomeException e) {
    LOG.error("...", e);
}

I'm well aware that there are exceptions to this general rule, but in a large code base, typically this is what you do want, so in my experience enforcing this helps not having lost exceptions in production logs; and experienced senior developers would @SuppressFBWarnings for the exceptional cases when it's not wanted.

Or does a check similar to this already exist in core FindBugs or other quality control tools?

odl-github pushed a commit to opendaylight/infrautils that referenced this issue Apr 24, 2018
if it's "CRITICAL", it most probably better be LOG.error() instead info,
and it would REALLY help to have the cause included; see also
KengoTODA/findbugs-slf4j#74.

actually, perhaps this really should re-throw... let me propose that in
a follow-up change, and we can discuss if there is any reason to only go
half the way or merge both steps.

Change-Id: Ie824d8e8b1155c6bfd604dd83da2969c4d9cdb33
Signed-off-by: Michael Vorburger <[email protected]>
odl-github pushed a commit to opendaylight/docs that referenced this issue Apr 24, 2018
* Update docs/submodules/infrautils from branch 'master'
  to 61f3a374264b8e6c6c0d8bce6b05431c77fa7a06
  - fix logging of (supposedly) "CRITICAL" condition
    
    if it's "CRITICAL", it most probably better be LOG.error() instead info,
    and it would REALLY help to have the cause included; see also
    KengoTODA/findbugs-slf4j#74.
    
    actually, perhaps this really should re-throw... let me propose that in
    a follow-up change, and we can discuss if there is any reason to only go
    half the way or merge both steps.
    
    Change-Id: Ie824d8e8b1155c6bfd604dd83da2969c4d9cdb33
    Signed-off-by: Michael Vorburger <[email protected]>
odl-github pushed a commit to opendaylight/releng-autorelease that referenced this issue Apr 24, 2018
* Update infrautils from branch 'master'
  to 61f3a374264b8e6c6c0d8bce6b05431c77fa7a06
  - fix logging of (supposedly) "CRITICAL" condition
    
    if it's "CRITICAL", it most probably better be LOG.error() instead info,
    and it would REALLY help to have the cause included; see also
    KengoTODA/findbugs-slf4j#74.
    
    actually, perhaps this really should re-throw... let me propose that in
    a follow-up change, and we can discuss if there is any reason to only go
    half the way or merge both steps.
    
    Change-Id: Ie824d8e8b1155c6bfd604dd83da2969c4d9cdb33
    Signed-off-by: Michael Vorburger <[email protected]>
@vorburger
Copy link
Contributor Author

@KengoTODA would you welcome (and merge) a PR for this?

@seanf
Copy link

seanf commented Aug 3, 2018

As far as I'm concerned, if you catch a Throwable, in general you should either log it or rethrow it (perhaps wrapped in another Throwable). But either one is fine.

Is the idea here this? "If there's a log statement, and it's inside a catch block, it must log the exception." So we're not checking every catch block, just the ones which contain logging? And, more generally, we're not checking for rethrows?

If so, that definitely sounds like a good check, but it doesn't seem like it covers everything suggested in spotbugs/discuss#49.

@vorburger
Copy link
Contributor Author

@seanf just FYI I'm holding further comments & contributions to this project until my #72 moves. I can only encourage you to raise a PR yourself for a check like this to cover everything suggested in spotbugs/discuss#49. PS: I like your (Commodore) logo!

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

No branches or pull requests

2 participants