apache / apache/gravitino

[Improvement] Skip building error stack traces when includeErrorStackTrace is disabled

Open
#13,108 1 comment 0 reactions 1 assignee Claimed by @nevzheng View on GitHub
improvement
Dominant language
Java
Stars
3.2k
Forks
935
Avg merge
1d 15h
Merged PRs (30d)
315

Description

### What would you like to be improved?

With `gravitino.server.webserver.includeErrorStackTrace=false`, error responses omit the `stack` field, but the server still builds the stack text for every failed request. `ErrorResponse`'s factory methods that take an exception call `getStackTrace(throwable)` unconditionally (formatting the stack into a string and splitting it into lines), and the `stack` field is only dropped when the response is written. Disabling the setting therefore saves response bytes but not the CPU and allocation cost of formatting stacks, which can be deep for exceptions with several causes. The JVM captures the stack when the exception is created either way; the avoidable cost is formatting it into text.

This was raised in review of #13057 (https://github.com/apache/gravitino/pull/13057#discussion_r3976730059) and deferred there, because that PR changes what error responses display, not how they are built.

The desired outcome is that failed requests don't pay to format stack traces when the setting is off, and that responses are unchanged in both modes.

### How should we improve?

**Options:**

- **A. Build the stack lazily in `ErrorResponse`:** keep the exception on the response (excluded from JSON and `equals`) and format the stack only when `getStack()` is read. With the setting off, the response writer skips `stack`, so the stack is never formatted. One file in `common/`, with no changes at call sites. Watch-outs: Lombok `equals`/`hashCode` and `toString` must not force formatting, and the response holds the exception until it is written.
- **B. Pass the setting to every place that builds an error response:** about 135 `Utils.*` calls in the server plus direct `ErrorResponse.*` calls, or give the shared `ErrorResponse` class access to server config. Larger, easy to miss call sites, and moves enforcement away from the single response-writing point.
- **C. Format the stack in the server's response writer:** `ErrorResponse` carries only the exception, and a server-side serializer next to `ObjectMapperProvider` formats `stack` only when the setting is on. Keeps the shared class a plain data holder, at the cost of more wiring than A.
- **D. A process-wide switch in `ErrorResponse`,** set at server startup. A very small change, but global mutable state in shared code: tests must reset it, and the switch applies to every user of `ErrorResponse` in the JVM rather than following one server's configuration.
- **E. Cap stack depth when stacks are enabled** (frames or causes). Reduces cost when the setting is on; it could be combined with A or C but does not address the disabled case alone.
- **F. Leave as is:** the cost applies only to failed requests and is small next to creating, handling and logging the exception.

Open question for triage: is the cost measurable enough to justify a change? Profiling the error path before choosing would answer that.

Related: #12728, #13057, #13107.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.