-
Notifications
You must be signed in to change notification settings - Fork 10
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
[Profiling/Build] Add crashtracking feature for windows (event it's not ready yet) #613
[Profiling/Build] Add crashtracking feature for windows (event it's not ready yet) #613
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #613 +/- ##
=======================================
Coverage 73.13% 73.13%
=======================================
Files 252 252
Lines 35959 35959
=======================================
Hits 26300 26300
Misses 9659 9659
|
BenchmarksComparisonBenchmark execution time: 2024-09-11 12:01:04 Comparing candidate commit 302f257 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 49 metrics, 2 unstable metrics. scenario:benching string interning on wordpress profile
scenario:concentrator/add_spans_to_concentrator
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
BaselineOmitted due to size. |
9fcbe37
to
feab5ef
Compare
feab5ef
to
d68c437
Compare
061205f
to
2d9e34e
Compare
d68c437
to
e0ba478
Compare
e0ba478
to
b41b28e
Compare
21b6e81
to
3dbd318
Compare
3dbd318
to
302f257
Compare
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.
It's hard for most of us to try this out without access to a windows machine, but on the other hand the change does look reasonable + it has a really tiny blast radius, so I see no reason not to merge it :)
rm -rf examples/ffi/build_dll | ||
mkdir examples/ffi/build_dll | ||
cd examples/ffi/build_dll | ||
cmake -S .. -DDatadog_ROOT=$LIBDD_OUTPUT_FOLDER -DVCRUNTIME_LINK_TYPE=DLL |
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.
Minor: Is this getting cached from run to run that it needs to clean the folder? 👀
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.
To be honest I do not know. But I observed FFI steps failing: symbolizer
could not run because libdatadog_profiling_ffi.so
was missing (there is no libdatadog_profiling_ffi.so
anymore, it's renamed into libdatadog_profiling.so
When I added this line, the issue went away. (mainly for the ubuntu-latest ones)
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.
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.
The package is run on every PR in Gitlab. At least, if files were missing or the names have changed, it would have failed. I plan to add tests:
|
What does this PR do?
Since #551, crashtracker has its own header
crashtracker.h
. This header is not part of the nuget package and prevent compilation on windows.Motivation
Make sure we can compile in Visual Studio.
Additional Notes
Waiting for #611
Anything else we should know when reviewing?
How to test the change?
Downloaded the nuget package (from the gitlab job) and checked that :
crashtracker.h
is presentddog_crasht_demangle
symbol is present