[crashtracking]: upgrade libdatadog for crashtracking to 26.0.0#61
Conversation
szegedi
left a comment
There was a problem hiding this comment.
Generally looks good to me; as long as the tests pass it's okay. I also had a fix for that crypto initialization thing in the top commit in the https://github.com/DataDog/libdatadog-nodejs/commits/szegedi/crashtracker-18/ branch, is that no longer needed?
Hmmm I don't have context on this; i'll investigate. Yup, can remove the crypto init |
Add pre-test script to build workflow for artifact handling
…taDog/libdatadog-nodejs into gyuheon0h/crashtracking-libdd-upgrade
rochdev
left a comment
There was a problem hiding this comment.
A few questions but no blocker!
* Upgrade to v26 and fix compat issues + serialization workaround * Serde + napi demo * remove demo code; keep commit for history * clean up * members being weird * Remove crypto init and make test resilient * Include pre-test script in build.yml Add pre-test script to build workflow for artifact handling * Remove crypto init * Revert build yml file
* Upgrade to v26 and fix compat issues + serialization workaround * Serde + napi demo * remove demo code; keep commit for history * clean up * members being weird * Remove crypto init and make test resilient * Include pre-test script in build.yml Add pre-test script to build workflow for artifact handling * Remove crypto init * Revert build yml file
* Upgrade to v26 and fix compat issues + serialization workaround * Serde + napi demo * remove demo code; keep commit for history * clean up * members being weird * Remove crypto init and make test resilient * Include pre-test script in build.yml Add pre-test script to build workflow for artifact handling * Remove crypto init * Revert build yml file
What does this do
Upgrades libdatadog for crashtracking to v26
When passing
signals: []to crashtracker, signal handlers were not installed, causing crashes to besilently ignored.
This upgrade was delayed due to issue described here: #54
Root Cause:
NAPI's
env.from_js_value()uses serde deserialization which bypassesCrashtrackerConfiguration::new().The constructor converts empty signals to default signals (SIGBUS, SIGABRT, SIGSEGV, SIGILL), but this
logic never runs when deserializing from JavaScript.
Commit with reproducible issue
Solution:
Added
apply_default_signals()helper incrates/crashtracker/src/lib.rsthat detects empty signals afterdeserialization and reconstructs the config using
new()to apply defaults. Applied in bothinit()andupdate_config().Additional notes:
We could have just bypassed this by explicitly listing out signal numbers, but the point is that we want the tracer/application actually running the crashtracker to be able to configure this.
Testing:
Crashtracking test passes with
signals: []- crashes are now correctly captured and reported.Ticket: https://datadoghq.atlassian.net/jira/software/c/projects/PROF/boards/11?selectedIssue=PROF-13449