profiler: use reqwest based exporter #1383
Closed
Conversation
This file contains hidden or 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
danielsn
force-pushed
the
dsn/r-and-d-exporter
branch
2 times, most recently
from
9837dcd to
a529caa
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1383 +/- ##
==========================================
+ Coverage 70.96% 71.11% +0.14%
==========================================
Files 400 401 +1
Lines 63691 63875 +184
==========================================
+ Hits 45199 45425 +226
+ Misses 18492 18450 -42
🚀 New features to boost your workflow:
|
BenchmarksComparisonBenchmark execution time: 2025-12-11 06:34:12 Comparing candidate commit 977d607 in PR branch Found 14 performance improvements and 1 performance regressions! Performance is the same for 42 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/ 3782-8224-6310-005
scenario:credit_card/is_card_number/ 378282246310005
scenario:credit_card/is_card_number/378282246310005
scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/ 378282246310005
scenario:credit_card/is_card_number_no_luhn/378282246310005
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
scenario:sql/obfuscate_sql_string
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
Group 13
Group 14
Group 15
Group 16
Group 17
Group 18
Group 19
BaselineOmitted due to size. |
danielsn
force-pushed
the
dsn/r-and-d-exporter
branch
from
a529caa to
0c12dc1
Compare
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-apple-darwin
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-apple-darwin
x86_64-unknown-linux-gnu
|
danielsn
force-pushed
the
dsn/r-and-d-exporter
branch
3 times, most recently
from
0e5a4d8 to
f558ac5
Compare
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected * Fix with Cursor requires Datadog plugin ≥v2.17.0 🔗 Commit SHA: 977d607 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
danielsn
force-pushed
the
dsn/r-and-d-exporter
branch
from
3061ae7 to
884a217
Compare
danielsn
force-pushed
the
dsn/r-and-d-exporter
branch
from
884a217 to
9f79fca
Compare
- Add cfg_attr to allow unused_mut on non-Unix platforms - Initialize request_url directly to avoid needless_late_init warning
…ing Tokio runtime The spawn_dump_server function is called from FFI contexts where a Tokio runtime may not be available. Converting it to use std::net::UnixListener and std::thread::spawn instead of Tokio async primitives allows it to work in any context. This fixes the test failure in reqwest_exporter::tests::test_file_endpoint.
The cbindgen rename value should not include the 'struct' keyword because cbindgen automatically adds it when generating opaque struct typedefs. Having 'struct' in the rename caused duplicate 'struct' keywords in the generated C header, resulting in compilation errors.
danielsn
force-pushed
the
dsn/r-and-d-exporter
branch
from
701948c to
242f8f8
Compare
Member
|
It feels like this will merge conflict with #1382 later 😉 |
Member
|
So I'm kinda curious about what's the objective/expected gains for this change and how we'll migrate to it. Specifically, reqwest still uses hyper AND tokio under the hood, so is the expected gain that our code becomes simpler? Also, the new exporter is getting added side-by-side with the old one. Is that a temporary thing, for validation? Could we tweak things a bit so that we can keep the same APIs, and maybe add a separate call/argument/env variable that toggles between both behaviors? |
Member
realFlowControl
left a comment
realFlowControl
left a comment
There was a problem hiding this comment.
The C/C++ bindings look okay to me, but as I won't consume them, @ivoanjo as one of the users should have a look at them as well.
Upgrade path for dd-trace-php looks easy, some tests are still failing.
The big thing for me: I am really not 100% sure anymore because this all started a long time ago and we seemed to have missed to chance to write a RFC for it. But wasn't the initial idea of switching away from Hyper bound to the idea of getting rid of Tokio? Or was it just because of the Hyper upgrade path being horrible and reqwest offering a nice wrapper around that?
TLDR: could you add to the description which problem we are solving?
Comment on lines
140
to
142
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()?; |
Member
There was a problem hiding this comment.
This creates a new Tokio runtime every time we do an export (~every 60 seconds) and throws it away on return of this function. I do not have any numbers but this feels heavy and I was wondering if we could "cache" the instance and re-use it.
| // Create a tokio runtime for this blocking call | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()?; |
Member
There was a problem hiding this comment.
(same as for the C-Bindings): This creates a new Tokio runtime every time we do an export (~every 60 seconds) and throws it away on return of this function. I do not have any numbers but this feels heavy and I was wondering if we could "cache" the instance and re-use it.
| } | ||
|
|
||
| /// Build and send a profile. Returns the HTTP status code. | ||
| pub async fn send( |
Member
There was a problem hiding this comment.
The current Hyper based ProfileExporter in
libdatadog/libdd-profiling/src/exporter/mod.rs
Lines 340 to 348 in 67d88eb
exposes this functions as sync/blocking one. Basically it had the Tokio runtime as an invariant and with this change, a consumer of this API would need to bring their own Tokio runtime. I guess this is fine, just wanted to make sure this is on purpose.
For the
dd-trace-php that would mean that we would have Tokio than as a first class dependency and not just a dependency of a dependency anymore.
Contributor
I don't want to stick my nose into profiling business, but just thinking about this from a wider perspective - Isn't the lack of windows named pipe support a dealbreaker for broader adoption of reqwest? Is it worth it for profiling to use a different library than others? |
Contributor
Author
Windows named pipe support is coming soon seanmonstar/reqwest#2789 |
Contributor
Awesome! Then ignore everything I said. |
Contributor
morrisonlevi
left a comment
morrisonlevi
left a comment
There was a problem hiding this comment.
I've been interrupted by my kids' evening activities, so I haven't finished, but here's my partial review.
Contributor
There was a problem hiding this comment.
./build-and-run-profiling.sh: Permission denied
Looks like there's a permissions issue, or launch it with bash instead.
| "Handle_EncodedProfile" = "ddog_prof_EncodedProfile" | ||
| "Result_HandleEncodedProfile" = "ddog_prof_EncodedProfile_Result" | ||
|
|
||
| "CancellationToken" = "struct ddog_OpaqueCancellationToken" |
Contributor
There was a problem hiding this comment.
I'm curious, did you just stumble across this in your editor or some other way? If this PR doesn't merge for whatever reason, this should probably get pulled out in merged in separately.
Comment on lines
+6
to
+7
| # Usage: ./build-and-run.sh <crate-name> <example-name> | ||
| # Example: ./build-and-run.sh libdd-profiling profiling |
Contributor
There was a problem hiding this comment.
On macOS, I'm getting an error:
% cd libdatadog/examples/cxx
% ./build-and-run.sh libdd-profiling profiling
🔨 Building libdd-profiling with cxx feature...
warning: /Users/levi.morrison/go/src/github.com/morrisonlevi/libdatadog_php_profiler/libdatadog/libdd-crashtracker-ffi/Cargo.toml: unused manifest key: target.cfg(windows).features
Finished `release` profile [optimized + debuginfo] target(s) in 0.21s
🔍 Finding CXX bridge headers...
📁 CXX include: target/release/build/libdd-profiling-9f264d581b94506e/out/cxxbridge/include
📁 CXX crate: target/release/build/libdd-profiling-9f264d581b94506e/out/cxxbridge/crate
📁 Rust CXX: target/release/build/cxx-ec62046ac43cc104/out
🔨 Finding libraries...
❌ Error: Could not find libdd-profiling library at $srcdir/libdatadog/target/release/liblibdd_profiling.aNotice the filename liblibdd_profiling.a, with lib repeated twice.
examples/ffi/reqwest_exporter.cpp
Outdated
| #include <memory> | ||
| #include <thread> | ||
|
|
||
| static ddog_CharSlice to_slice_c_char(const char *s) { return {.ptr = s, .len = strlen(s)}; } |
Contributor
There was a problem hiding this comment.
Does the cxx-ified version not end up with the DDOG_CHARSLICE_C and DDOG_CHARSLICE_C_BARE macros? Shouldn't need this unless you need an actual function for something.
Comment on lines
+48
to
+60
| pub struct File<'a> { | ||
| pub name: &'a str, | ||
| pub bytes: &'a [u8], | ||
| } | ||
|
|
||
| impl<'a> From<super::File<'a>> for File<'a> { | ||
| fn from(file: super::File<'a>) -> Self { | ||
| Self { | ||
| name: file.name, | ||
| bytes: file.bytes, | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
I don't see any differences between File and super::File, they are both:
pub struct File<'a> {
pub name: &'a str,
pub bytes: &'a [u8],
}What's the reason for duplicating it?
Contributor
Author
|
Supplanted by #1412 |
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?
Uses reqwest to send profiles instead of hyper.
Motivation
The hyper code has to handle a lot of lower level issues and is therefore more complex.
Additional Notes
Most of the crazy LOC change is in licence third party
How to test the change?
Describe here in detail how the change can be validated.