Skip to content

Conversation

@pkoenig10
Copy link
Member

Before this PR

Users can't create a QosException with a cause. This means we lose the cause information is lost if a caught exception is re-thrown as a QosException.

After this PR

Users can create a QosException with a cause.

@changelog-app
Copy link

changelog-app bot commented Jul 15, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

QosException can now be created with a cause.

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from dansanduleac July 15, 2020 23:42
@markelliot
Copy link

What would a consumer do with this information? If it’s for service debugging, I think we’d probably prefer logging the relevant cause information at the catch site rather than depending on the outbound server logging recording cause information.

@pkoenig10
Copy link
Member Author

pkoenig10 commented Jul 15, 2020

Exception causes are not included in SerializableError and never reach consumers. This is purely for logging the exception.

I think we’d probably prefer logging the relevant cause information at the catch site rather than depending on the outbound server logging recording cause information.

We support providing a cause for ServiceException and for the exceptions in https://github.com/palantir/safe-logging. It seems odd to me that the rationale here would not be the same.

Sure, explicitly logging at the catch site is a solution. But it just seems unnecessarily cumbersome.

Do you have a specific concern with exposing this functionality? Is there some behavioral or API design risk?

@carterkozak
Copy link
Contributor

I also prefer retaining causes and providing full context at the top level (service exception handler in this case) unless handling an exception is expected to take a great deal of time. That way we don't have to worry about updating N projects log statements if we decide qos exceptions should be logged at info instead of debug. catch + log + throw is an antipattern because it's harder to piece together full context at a glance. Take the active project to aggregate internal error traces for example, the resulting stack traces are more actionable if they provide the full cause chain.

@markelliot
Copy link

Ok.

@bulldozer-bot bulldozer-bot bot merged commit a8389e0 into develop Jul 16, 2020
@bulldozer-bot bulldozer-bot bot deleted the pkoenig10/cause branch July 16, 2020 15:58
@svc-autorelease
Copy link
Collaborator

Released 2.15.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants