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

Log suppressed exceptions in MillisecondPrecisionSyslogAppender #25

Merged
merged 1 commit into from
Nov 11, 2019

Conversation

totalorder
Copy link

This adds support for logging suppressed exceptions. Without this change they are silently ignored contrary to a regular throwable.printStackTrace() which does print them.
Inspiration has been taken from the upstream implementation in another appender: https://github.com/qos-ch/logback/blob/master/logback-classic/src/main/java/ch/qos/logback/classic/pattern/RootCauseFirstThrowableProxyConverter.java#L33

@totalorder
Copy link
Author

Is there a reasonably easy way to write tests for this class? I don't even know where to start.
In the mean time we are testing this by shadowing it in our service. Testing in production 🤠 https://ghe.spotify.net/search-platform/search-api/pull/1116

@totalorder
Copy link
Author

@danielnorberg @pettermahlen Please take a look

Copy link
Member

@pettermahlen pettermahlen left a comment

Choose a reason for hiding this comment

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

LGTM, perhaps give the shadow testing a couple of days before merge?

@totalorder
Copy link
Author

Sounds good! We'll run it in production for a few days before merge. I want to make sure google stackdriver manages to group the stacktrace correctly too.

@totalorder
Copy link
Author

@pettermahlen We have been running this in production for ~10 days and it seems to work well. Would you mind merging it?

@pettermahlen pettermahlen merged commit 79ad924 into spotify:master Nov 11, 2019
@totalorder
Copy link
Author

@pettermahlen Thanks! I saw the last release was ~1 year ago. Is there still someone around who knows how to make a new release of this package? :)

@pettermahlen
Copy link
Member

@totalorder I will dare it, any time now :) Not sure that the current laptop is set up for it, but we'll see. Not sure I'll get to it today, a lot of meetings.

@pettermahlen
Copy link
Member

2.2.7 is now out and available via Maven Central.

@totalorder
Copy link
Author

Awesome! You are a brave soul

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

Successfully merging this pull request may close these issues.

None yet

2 participants