-
Notifications
You must be signed in to change notification settings - Fork 373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NO-TICKET] Upgrade to libdatadog 14.1 #4082
Conversation
**What does this PR do?** This PR upgrades the libdatadog dependency to the latest 14.1 release. **Motivation:** This PR fixes the issues we uncovered during testing of 14.0 in #4065: specifically, that triggering a stack overflow with native code would result in incorrect behavior (a segfault, not a stack overflow exception) when crashtracking was enabled. **Additional Notes:** N/A **How to test the change?** Our existing test coverage includes libdatadog testing. The native code stack overflow is a bit of a pain to trigger because GCC seems to like optimizing away such things. What I did was compile the profiler native extension with `export DDTRACE_DEBUG=true` (remember to run clean first!), and then adding the following trivial stack overflow: ```diff diff --git a/ext/datadog_profiling_native_extension/profiling.c b/ext/datadog_profiling_native_extension/profiling.c index 96637cd110..4ec5505147 100644 --- a/ext/datadog_profiling_native_extension/profiling.c +++ b/ext/datadog_profiling_native_extension/profiling.c @@ -38,6 +38,12 @@ static VALUE _native_enforce_success(DDTRACE_UNUSED VALUE _self, VALUE syserr_er static void *trigger_enforce_success(void *trigger_args); static VALUE _native_malloc_stats(DDTRACE_UNUSED VALUE _self); +static VALUE blow_stack(DDTRACE_UNUSED VALUE _self) { + fprintf(stderr, "."); + blow_stack(Qnil); + return Qnil; +} + void DDTRACE_EXPORT Init_datadog_profiling_native_extension(void) { VALUE datadog_module = rb_define_module("Datadog"); VALUE profiling_module = rb_define_module_under(datadog_module, "Profiling"); @@ -72,6 +78,8 @@ void DDTRACE_EXPORT Init_datadog_profiling_native_extension(void) { rb_define_singleton_method(testing_module, "_native_trigger_holding_the_gvl_signal_handler_on", _native_trigger_holding_the_gvl_signal_handler_on, 1); rb_define_singleton_method(testing_module, "_native_enforce_success", _native_enforce_success, 2); rb_define_singleton_method(testing_module, "_native_malloc_stats", _native_malloc_stats, 0); + + rb_define_singleton_method(testing_module, "blow_stack", blow_stack, 0); } ```
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4082 +/- ##
==========================================
+ Coverage 97.72% 97.73% +0.01%
==========================================
Files 1338 1338
Lines 80251 80251
Branches 4017 4017
==========================================
+ Hits 78425 78434 +9
+ Misses 1826 1817 -9 ☔ View full report in Codecov by Sentry. |
Sounds like TCO? |
Possibly, but I didn't check so I didn't want to make claims regarding that ;) |
BenchmarksBenchmark execution time: 2024-11-07 10:39:36 Comparing candidate commit 0fa0e1f in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 29 metrics, 2 unstable metrics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for all the testing on this!
What does this PR do?
This PR upgrades the libdatadog dependency to the latest 14.1 release.
Motivation:
This PR fixes the issues we uncovered during testing of 14.0 in #4065: specifically, that triggering a stack overflow with native code would result in incorrect behavior (a segfault, not a stack overflow exception) when crashtracking was enabled.
Change log entry
Upgraded libdatadog dependency to version 14.1
Additional Notes:
N/A
How to test the change?
Our existing test coverage includes libdatadog testing.
The native code stack overflow is a bit of a pain to trigger because GCC seems to like optimizing away such things.
What I did was compile the profiler native extension with
export DDTRACE_DEBUG=true
(remember to run clean first!), and then adding the following trivial stack overflow: