RemoteException.getMessage includes SerializableError parameters #633
Conversation
Generate changelog in
|
In connected environments this makes no difference because RemoteException.getMessage is never called, only getLogMessage. In such cases the error instance identifier is sufficient to search for the original failure information, however service authors without safe-logging infrastructure are unlikely to have access to remote instance logging and should be presented with the failure parameters in local logging. The message is computed lazily, very slightly reducing overhead in environments that don't use the unsafe message, while providing additional actionable signal to others.
carterkozak
force-pushed
the
ckozak/unsafelog_remoteexception_args
branch
from
454d5d6
to
0bf9c2e
Compare
|
Just want to flag that we previously rejected this change in #337. I'm not opposed to making this change (I think I'd actually prefer it), I just want to ensure that we're now willing to accept the tradeoffs here. |
|
Yep yep, Felix reported a case where an issue was difficult to debug due to lack of insight into the server-side of the problem, recorded in a non-safe-logging environment. I think that’s the upside we were missing last time. |
| .forEach((name, unsafeValue) -> | ||
| builder.append(name).append('=').append(unsafeValue).append(", ")); | ||
| // remove the trailing space | ||
| builder.setLength(builder.length() - 1); |
There was a problem hiding this comment.
In this case it's unlikely that the last character resulted in a buffer expansion, so reducing the length by 1 is a non-volatile write to the StringBuilder.count integer.
Joiner would work nicely if this didn't involve relatively complex prefix parameters -- we'd end up creating a bunch of intermediate strings.
|
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. |
| this.error = error; | ||
| this.status = status; | ||
| this.args = Collections.singletonList(SafeArg.of("errorInstanceId", error.errorInstanceId())); | ||
| } | ||
|
|
||
| @Override | ||
| public String getMessage() { | ||
| return message; | ||
| // This field is not used in most environments so the cost of computation may be avoided. | ||
| String messageValue = unsafeMessage; |
There was a problem hiding this comment.
Curious, why do we need the messageValue helper variable instead of doing something like:
if (unsafeMessage == null) {
unsafeMessage = renderUnsafeMessage();
}
return unsafeMessage;There was a problem hiding this comment.
Optimization, not likely a measurable difference, but we’re reading the field value once instead of twice. I find it cleaner to assert on the same value that’s returned as well (avoids a class of race condition bugs)
| : String.format("RemoteException: %s (%s)", error.errorCode(), error.errorName()); | ||
| this.message = this.stableMessage + " with instance ID " + error.errorInstanceId(); | ||
| ? "RemoteException: " + error.errorCode() | ||
| : "RemoteException: " + error.errorCode() + " (" + error.errorName() + ")"; | ||
| this.error = error; | ||
| this.status = status; | ||
| this.args = Collections.singletonList(SafeArg.of("errorInstanceId", error.errorInstanceId())); | ||
| } | ||
|
|
||
| @Override | ||
| public String getMessage() { |
There was a problem hiding this comment.
Should we also add the new safety annotations to this class?
There was a problem hiding this comment.
Wouldn’t hurt, although we already consider throwable.getMessage unsafe across the board
|
Released 2.25.0 |
In connected environments this makes no difference because
RemoteException.getMessage is never called, only getLogMessage.
In such cases the error instance identifier is sufficient to
search for the original failure information, however service
authors without safe-logging infrastructure are unlikely to have
access to remote instance logging and should be presented with
the failure parameters in local logging.
The message is computed lazily, very slightly reducing overhead
in environments that don't use the unsafe message, while providing
additional actionable signal to others.
After this PR
==COMMIT_MSG==
RemoteException.getMessage (unsafe message) includes SerializableError parameters. RemoteException.getLogMessage (safe) is not impacted.
==COMMIT_MSG==