generate core dump file for easier debug

[Besroy]

No description provided.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.03%. Comparing base (370c772) to head (171e01a).
⚠️ Report is 32 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   64.29%   61.03%   -3.27%     
==========================================
  Files          72       74       +2     
  Lines        4406     4863     +457     
  Branches      555      663     +108     
==========================================
+ Hits         2833     2968     +135     
- Misses       1327     1631     +304     
- Partials      246      264      +18     

Components	Coverage Δ
AuthManager	77.77% <ø> (ø)
Cache	40.50% <84.69%> (+10.56%)	⬆️
FDS	71.19% <100.00%> (+0.07%)	⬆️
FileWatcher	58.51% <60.00%> (+2.26%)	⬆️
Flip	65.34% <ø> (ø)
gRPC	76.04% <64.70%> (-1.01%)	⬇️
Logging	15.30% <3.62%> (-14.88%)	⬇️
Metrics	80.54% <ø> (+0.33%)	⬆️
Options	100.00% <ø> (ø)
Setting	56.79% <ø> (ø)
StatusObject	73.83% <ø> (ø)
Utility	83.47% <100.00%> (+0.75%)	⬆️
Version	95.83% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[xiaoxichen]

lgtm but wondering the rational behind original code. waiting for brain to confirm

[szmyd]

lgtm but wondering the rational behind original code. waiting for brain to confirm

IIRC Bryan Zimmerman wrote this stacktrace logic. My guess is that the intent was to avoid any kind of circular logic when doing normal process termination (as std::exit() does) using _exit instead. Raising an exception will cause this thread to unwind up to the top and then cause another Signal won't it?

I would rather investigate migrating to libcpptrace (https://github.com/jeremy-rifkin/cpptrace) like the BayDB project is using and just dump a stack from here rather than rely on core files.

[xiaoxichen]

I see we have backtrace in the log? not sure if it is using cpptrace...

Previous code was trying to ignore SIGABORT for some reason :) As the signal handler all reset to default so it wont cause another signal .

[Besroy]

I see we have backtrace in the log? not sure if it is using cpptrace...

Previous code was trying to ignore SIGABORT for some reason :) As the signal handler all reset to default so it wont cause another signal .

Agreed. We have backtrace information in the logs (without using cpptrace), but it's insufficient for thorough debugging. We still require a core dump file to access more detailed information. Raising signals such as SIGABRT, SIGFPE, SIGSEGV, and SIGILL will not lead to circular logic, as the signal handlers are reset to their default behavior, which is to terminate the process and generate a core dump file. @szmyd pls correct me if I'm wrong.

[szmyd]

We also at some point starting using minidumps since our cores were absolutely massive (many GiBs). Does this impact that in anyway? @raakella1 knows a little more about this than myself maybe, just want to make sure we don't start blowing out the local PVC on crashes in production.

[raakella1]

We disabled collecting the traditional stacktrace and use minidump instead for the release builds because it takes a very long time for the process to exit waiting for all the threads to dump the stacktrace (up to an hour and sometimes stuck even forever) which is not acceptable for release builds.
Only debug builds dump the stacktrace. check https://github.com/eBay/sisl/blob/master/src/logging/stacktrace.cpp#L35
The large size of the core files is also a concern in production. It will compete for space with the log files. Honestly, for release builds, (even if we build Rel with Deb Info) there is no useful information in the core file.
You can maybe just dump the core file for debug builds
@Besroy @xiaoxichen @szmyd

[Besroy]

@raakella1 I completely understand and appreciate your concerns regarding the size and generation time of coredump files. However, during SH's testing process, we observed that even the corefile in RelWithDebugInfo has proven to be very helpful for debugging. This document highlights several issues that were successfully debugged using core files.

Would it be possible to consider adding a configuration option where coredump generation is disabled by default? This way, SM or other users could enable it by modifying the configuration if needed, ensuring minimal impact on other projects.

[raakella1]

@Besroy Yes, config option seems like a good middle ground.

[Besroy]

Hi @raakella1 I add an option to enable core dump, please take a look, thanks!