-
Notifications
You must be signed in to change notification settings - Fork 16
refactor(profiling)!: use reqwest instead of hyper for exporter #1444
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
Conversation
6743cb4 to
ec85a33
Compare
BenchmarksComparisonBenchmark execution time: 2026-01-14 21:18:34 Comparing candidate commit 8358888 in PR branch Found 16 performance improvements and 2 performance regressions! Performance is the same for 39 metrics, 2 unstable metrics. scenario:benching serializing traces from their internal representation to msgpack
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:normalization/normalize_name/normalize_name/good
scenario:sdk_test_data/rules-based
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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1444 +/- ##
==========================================
- Coverage 71.30% 71.28% -0.02%
==========================================
Files 413 416 +3
Lines 66157 66757 +600
==========================================
+ Hits 47172 47591 +419
- Misses 18985 19166 +181
🚀 New features to boost your workflow:
|
ec85a33 to
5b427a1
Compare
5b427a1 to
779821f
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
|
taegyunkim
left a comment
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.
Overall looks good to me, though my knowledge on Rust/async/tokio is somewhat limited.
| /// [`send`]: ProfileExporter::send | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn send_blocking( | ||
| &mut self, |
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.
One question I had in mind was why would this need to take self as mutable.
It's modifying self.runtime, and consulting with claude suggested using OnceLock but turns out that it won't be thread-safe given that we use new_current_thread().
Correct me, if my understanding is wrong.
morrisonlevi
left a comment
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.
I need to review in depth still but I noticed the artifacts came up in size a fair bit. Maybe there's something we can do to trim it back down? Maybe features we can disable?
99bfb90 to
bec5c87
Compare
morrisonlevi
left a comment
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.
I haven't tried it out with PHP yet, but I did notice something was lost from main.
38b8ad9 to
c8719cb
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
|
This change fixes a customer affecting fork-safety issue on python, so going ahead and will look at binary size later. |
c8719cb to
d3050db
Compare
What does this PR do?
Uses reqwest instead of hyper for the profiler exporter.
Motivation
Using a high-level library rather than low-level networking makes the code clearer.
Additional Notes
Technically a breaking change since we change Rust APIs but we don't affect the FFI at all.
How to test the change?
Existing tests, plus a new end to end set of tests for the different endpoint types