Skip to content

Conversation

ysiraichi
Copy link
Collaborator

@ysiraichi ysiraichi commented Jul 19, 2025

This PR addresses the discussion in #9339 by enhancing error reporting with C++ and Status propagation stack traces. The main goal is to provide more informative error messages by including both C++ stacktraces and Status propagation stacktraces. This should help developers quickly pinpoint error origins and understand how errors propagate through the system, simplifying debugging.

Key Changes:

  • Moved MaybeWithLocation and MaybeWithNewMessage to the status_internal namespace, as they are intended for internal use only.
  • Implemented the use of Status payload for separate storage of the status propagation stacktrace.
  • Added functionality to dump the C++ stacktrace by calling tsl::CurrentStackTrace() immediately before throwing an exception.
  • Adapted existing tests in test_status_common.h to reflect these changes.
  • Introduced a new test, MaybeThrowWithErrorPropagationWithNewMessage, to demonstrate the format of the enhanced error messages.
  • Renamed testing global variables in test_status_common.h to adhere to the kVarName pattern.

Update: added the function name __FUNCTION__ to the status propagation trace. Lambda functions are recorded as operator().

Before:

  • No C++ stacktrace
  • Line 342, which propagates the status, is not mentioned
RuntimeError: New test error message (at ./test/cpp/test_status_common.h:335)
    From Error: Test error message (at ./test/cpp/test_status_common.h:329)

After:

RuntimeError: New test error message

Status Propagation Stacktrace:
    From: foo at ./test/cpp/test_status_common.h:329 (error: Test error message)
    From: bar at ./test/cpp/test_status_common.h:335 (error: New test error message)
    From: baz at ./test/cpp/test_status_common.h:342

C++ Stacktrace:
*** Begin stack trace ***
        ...
        __libc_start_main
*** End stack trace ***

Copy link
Collaborator

@zhanyong-wan zhanyong-wan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Collaborator

@ghpvnist ghpvnist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ysiraichi ysiraichi force-pushed the ysiraichi/dump-stacktraces branch from ec39b02 to 5b0aaf4 Compare July 24, 2025 12:49
Copy link
Collaborator

@zhanyong-wan zhanyong-wan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@ysiraichi ysiraichi force-pushed the ysiraichi/dump-stacktraces branch from 5b0aaf4 to e0427a2 Compare July 29, 2025 01:20
@ysiraichi
Copy link
Collaborator Author

@ghpvnist I have addressed your comments. Let me know if there's anything else you'd like to see in this PR.
Otherwise, could you approve it?

Copy link
Collaborator

@ghpvnist ghpvnist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ysiraichi ysiraichi merged commit 7aa466e into master Jul 29, 2025
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants