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

Deprecates ExceptionLoggingFilter and disables it by default #1633

Merged
merged 4 commits into from May 15, 2020

Commits on May 15, 2020

  1. Deprecates ExceptionLoggingFilter and disables it by default

    `ExceptionLoggingFilter` logs "Uncaught exception thrown" to error level
    when there is a synchronous exception not otherwise swallowed. This is a
    cure worse than the disease. This issue disables by default and the 3.x
    should end the years of problems it caused.
    
    Fixes #1347
    
    This was done IIUC to help @jfuerth who had log messages with trace IDs,
    except the uncaught one, in #714 (Sept 2017). They were formerly told to
    use `ExceptionHandler` and noted that didn't work in practice as it
    subverted their status code logic.
    
    Concretely, 4203566 "solved" the issue
    by having sleuth log all uncaught exceptions with the message
    "Uncaught exception thrown". This code was initially embedded in
    sleuth's and later pulled out to `ExceptionLoggingFilter` in 2.0.
    
    A few months later @brenuart noticed this "solution" had problems. For
    example, it caused the same exception to be logged twice. Bertrand also
    mentioned some faults in the implementation including edge cases like
    `ClientAbortException` not addressed by this. (In fact, there are even
    more problems: the implementation assumes async isn't in use!)
    Bertands concerns about this having surprising logging effects were
    dismissed as a non-issue. Bertrand raised #859 about glitches this
    caused, the status_code related ones being if statemented around, and
    it went quiet a while.
    
    A month later @oburgosm opened #902 (currently 5 thumbs up) asking to
    remove the "Uncaught exception thrown" or at least set it to debug.
    The response was if they don't like it, control their own tracing
    filter and the issue was closed.
    
    A month later, @nickcodefresh opened #966 confused about "Uncaught
    exception thrown" messages raised by sleuth, incidentally the
    `ClientAbortException` mentioned by @brenuart before. Because the
    logging came from Sleuth, it appeared they were zipkin errors, and was
    explained they weren't.
    
    A year later, @T3rm1 opened #1347 to remove the filter, or at least
    disable it by default. The response was first to set the Logger to OFF.
    A few days later the issue was closed as the submitter was told they
    were the first person to complain about this. 5 months later, another
    user rallied for support.
    
    In April 2020, #902 was re-touched by @kosciej who asked to please
    reconsider, mentioning it was not typical even in Spring to do this,
    for example 'org.springframework.web.filter' package. The impact of
    needing to specifically change apps to OFF the sleuth logger seemed
    harsh. The suggestion was instead of setting log config, to set a
    property instead `spring.sleuth.web.exception-logging-filter-enabled`,
    to `false` as that could also accomplish the goal of stopping
    "Uncaught exception thrown".
    
    A month later (yesterday) @MrHurniak commented again on #1347 with the
    usual complaint that it is redundant and confusing.
    Adrian Cole committed May 15, 2020
    Copy the full SHA
    73551ca View commit details
    Browse the repository at this point in the history
  2. other test

    Adrian Cole committed May 15, 2020
    Copy the full SHA
    160d48b View commit details
    Browse the repository at this point in the history
  3. shows FSH alternative and unwinds config spaghetti

    Adrian Cole committed May 15, 2020
    Copy the full SHA
    9f894aa View commit details
    Browse the repository at this point in the history
  4. try without auto at all

    Adrian Cole committed May 15, 2020
    Copy the full SHA
    81ca576 View commit details
    Browse the repository at this point in the history