Init CrashTracker support in the sidecar #535
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
2f6dcc3
to
e0e853b
Compare
BenchmarksComparisonBenchmark execution time: 2024-08-19 13:31:05 Comparing candidate commit ab216b8 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 50 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
BaselineOmitted due to size. |
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
e0e853b
to
26b5d49
Compare
This was referenced Jul 18, 2024
Library Vulnerabilities✅ No library vulnerabilities found (scanned 6652db2). |
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
2eec957
to
6652db2
Compare
danielsn
reviewed
Jul 18, 2024
crashtracker/src/crash_info.rs
Outdated
| } | ||
| self.upload_to_telemetry(config) | ||
|
|
||
| let rt = tokio::runtime::Builder::new_current_thread() |
There was a problem hiding this comment.
This is probably not signal safe, should update the comment above
There was a problem hiding this comment.
I removed the comment because I didn't know what to say...
If you have a suggestion, I'm all ears.
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn async_receiver_entry_point_unix_socket( |
There was a problem hiding this comment.
do we have a test for this
There was a problem hiding this comment.
It's tested by the "bin_tests" as both sync and async "entry_point_unix_socket" use this code path
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
bb52cd7
to
b13bb99
Compare
danielsn
reviewed
Jul 22, 2024
| /// Return the path of the crashtracker unix domain socket. | ||
| #[no_mangle] | ||
| #[allow(clippy::missing_safety_doc)] | ||
| pub unsafe extern "C" fn ddog_sidecar_get_crashtracker_unix_socket_path() -> ffi::CharSlice<'static> |
There was a problem hiding this comment.
Who is the owner of this string, and is expected to free it?
There was a problem hiding this comment.
The caller. I mean, it returns libc::malloc, so the caller can just free() it.
| let socket_path = crashtracker_unix_socket_path(); | ||
| let str = socket_path.to_str().unwrap_or_default(); | ||
|
|
||
| let size = str.len(); |
There was a problem hiding this comment.
Would it be cleaner to take a trip through https://doc.rust-lang.org/std/primitive.str.html#method.into_boxed_bytes
There was a problem hiding this comment.
At the cost of not being able to free() it in C code. This impl allows avoiding to need extra drop routines / relying on the specific allocator rust currently uses.
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
4ae9922
to
8431eb2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
==========================================
- Coverage 71.77% 71.77% -0.01%
==========================================
Files 237 238 +1
Lines 32871 32924 +53
==========================================
+ Hits 23594 23631 +37
- Misses 9277 9293 +16
|
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
2527cef
to
49a3786
Compare
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
49a3786
to
f9dfa04
Compare
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
ae9301a
to
fe64142
Compare
bwoebi
approved these changes
Aug 14, 2024
iamluc
force-pushed
the
luc/php-crashtracker
branch
2 times, most recently
from
f2ce221
to
2ae68b8
Compare
iamluc
force-pushed
the
luc/php-crashtracker
branch
from
2ae68b8
to
ab216b8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR introduces a CrashTracker receiver (via a Unix socket) in the sidecar.
As the sidecar uses the Tokio async framework, I had to convert a few functions from the
crashtrackercrate into async functions.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.