ServiceExceptions should retain errorInstanceId from causes

[carterkozak]

What happened?

Applications often catch and rethrow exceptions, for example, a service may catch RemoteException, and throw a more specific ServiceException with a better ErrorType using the RemoteException as a cause. Today this generates a second errorInstanceId despite being a continuation of the same error. Debugging the error by looking up the errorInstanceId becomes an O(n) operation rather than O(1) where a single key provides a veiw of the failure across systems.
The current behavior is perplexing because allowing RemoteExceptions to be reach conjure exception mappers does result in the desired behavior, however in some cases (think failures from an executor) rethrowing the original exception directly makes it harder to trace execution, creating a new exception on the thread calling Future::get allows us to follow the failure to an endpoint.

What did you want to happen?

ServiceException should traverse causes for an errorInstanceId before generating a new identifier.

[carterkozak]

@uschi2000 I'd appreciate your feedback on this.

[ferozco]

I'm not familiar with the context around the original decision to create new errorInstanceIds so I'm also curious to hear @uschi2000s thoughts. The proposal does seem nice from a debugging perspective and encourages better handling of remoteExceptions. We do lose the uniqueness of errorInstanceIds but I'm not sure that really bought us anything

[carterkozak]

I don't thinkerrorInstanceId is actually unique today because raw RemoteExceptions that propagate to our exception mappers are logged with their existing ID, not a new one.

[iamdanfox]

I think the intention behind errorInstanceId is just that since we're redacting so much stuff (e.g. the entire stack trace), we need to give people something reasonably unique they can search for to find the relevant stack trace on the server side.

As an alternative workflow, would searching for the traceId not find the whole chain of relevant errors in upstream errors?

[carterkozak]

I think the intention behind errorInstanceId is just that since we're redacting so much stuff (e.g. the entire stack trace), we need to give people something reasonably unique they can search for to find the relevant stack trace on the server side.

Yep, this is what I want to do!

As an alternative workflow, would searching for the traceId not find the whole chain of relevant errors in upstream errors?

it's a workaround, but has a few important drawbacks:

1. Not all causes have the same traceId, though we're trying
2. Some trace operations are very long and contain thousands of log lines. In many cases a single failure is recoverable and only a small slice of the operation, but it's still important to figure out what went wrong
3. It's difficult to pick out the right errorInstanceId when there are several present in a single trace. Understanding the causal chain is important, where some errors may be unrelated or recovered.

[carterkozak]

In the ticket that prompted this issue, one of the two errorInstanceIds does not have a traceId

[uschi2000]

The idealized original design, if I remember correctly, was to use errorInstanceIds as local identifiers for an exception living in a single JVM, and traceIds as correlation mechanism across different RPC actors. In that light, the fact that RemoteException errorInstanceIds get propagated is probably a bug (or oversight) rather than intention.

Academically, I would thus prefer we invest time in trace id coverage rather than plumbing error instance ids. (Counter-factually: why would be easier to ensure complete error instance id propagation than to ensure complete trace id propagation?)

[carterkozak]

The idealized original design, if I remember correctly, was to use errorInstanceIds as local identifiers for an exception living in a single JVM, and traceIds as correlation mechanism across different RPC actors. In that light, the fact that RemoteException errorInstanceIds get propagated is probably a bug (or oversight) rather than intention.

I had considered errorInstanceIds as identifiers for single errors, which can be propagated though several services, based on how RemoteException is used. The current implementations leave us in an undesirable middle-ground where I'd prefer to either remove propagation from RemoteException, or add propagation as described in this issue.
I imagine it would be painful to remove propagation from RemoteExceptions, especially given the conjure proxy service use case, where we're generally not concerned with errors from the proxy, just the client handling the final error, and the server which created the initial error. Resolving this issue would allow the proxy case to provide better service exceptions to its clients without sacrificing observability.

why would be easier to ensure complete error instance id propagation than to ensure complete trace id propagation?

Exception causes can be analyzed, and traced in source code. Tracing uses thread-local state that cannot, any library that erroneously/asymmetrically calls Tracers.completeSpan() results in a swath of execution without a traceId.
Second, tracing architecture allows code to explicitly run with a different traceId using Tracers.wrapWithNewTrace (among other utilities). RuntimeExceptions (RemoteException, ServiceException, et al) are not necessarily logged and handled in the same scope, resulting in logged with a different traceId from their causes and making it impossible to find causes using the traceId. In the simple case, not all executors/threads are aware of tracing, so new traceIds are generated when we make a call using a conjure client, and the trace ends when that call returns. Exceptions thrown are eventually logged, but have no traceId.
Finally, there is no bound on the size of a single trace, looking up failures by traceId can result in noise where our log aggregation tooling has a hard time finding the relevant time interval for the failure we're investigating an exception. We can break large operations into smaller operations, but that requires splitting traces and results in the problem from the second point.

[uschi2000]

OK, thanks for your explanation, Carter. Given the half-baked status quo, I have no religious sentiment towards either error instance ids being propagated or not.

[carterkozak]

Thanks, Rob.