Add parameters to RemoteException message

[pkoenig10]

Fixes #226

Before this PR

The implementations of RemoteException and ServiceException are not consistent:

• RemoteException does not implement SafeLoggable, ServiceException does
• RemoteException#getMessage does not include the parameters, ServiceException#getMessage does

After this PR

The implementations of RemoteException and ServiceException are more consistent:

• Both implement SafeLoggable. getLogMessage does not include any args/parameters.
• getMessage includes all args/parameters, including unsafe args.

This is slightly different than #227 because this PR adds the parameters to the RemoteException message.

[changelog-app]

Generate changelog in changelog/@unreleased

Type

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

Description

RemoteException now implements SafeLoggable with behavior identical to ServiceException.

Check the box to generate changelog(s)

• Generate changelog entry

[carterkozak]

Thoughts on this proposal? #353

[pkoenig10]

I think #353 was definitely a positive change.

But I would still like to have RemoteException and ServiceException implemented consistently. For example, ServiceException#getMessage includes the parameters while RemoteException#getMessage does not.

[rshkv]

Another argument for this: When propagating errors by re-throwing Remote- as ServiceExceptions, you no longer need to convert parameter maps to args.

[stale]

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

[pkoenig10]

This PR should stay open. @dansanduleac @ferozco can someone take a look at this PR?

[stale]

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

[pkoenig10]

@palantir/infrastructure any updates here?

[stale]

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

[iamdanfox]

Ok so I don't have the full context that motivated this PR (the inconsistency doesn't bother me too much), but I think the PR in its current state seems reasonable!

• I guess we'd end up spending a little more CPU on every RemoteException building up those strings (especially if there's a very large parameter), but this likely pales in comparison to the cost of the actual RPC.
• One slight concern is that if we start building strings for people I worry that this will lead to a proliferation of string parsing to get arguments out.
• Also this PR would increase the seriousness of any kind of data leak that includes RemoteException messages (but ideally we should have zero of these anyway).

@pkoenig10 could you provide a tiny bit more context of why you want this? (Just to help me get over my "if it ain't broke don't fix it" resistance). Also if there are any downsides others know of that I haven't listed above please shout otherwise let's just merge it.

[pkoenig10]

@iamdanfox there aren't any downsides other than the ones you've already listed. I opened this PR before #353 so my primary motivation was to make RemoteException safe loggable. It seemed like there were a couple different opinions on the implementation so I figured it would be best to make it consistent with ServiceException.

Looking back at this PR after rebasing on #353, it probably doesn't add much value because the args are all unsafe. I think @carterkozak makes a good argument for why he decided to just omit the args here:

conjure-java-runtime-api/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java

Lines 62 to 63 in 9be6496

	// RemoteException explicitly does not support arguments because they have already been recorded
	// on the service which produced the causal SerializableError.

At this point I'm pretty indifferent about merging vs closing this PR. Having the consistency would be nice, but introducing additional cost for little to no value seems a bit silly.

[iamdanfox]

OK cool - if there aren't big upsides then I think i'd prefer to just park this and leave things as they are to avoid unintended consequences.