perf(profiling): cache TLS in ProfileExporter::new#1619
Conversation
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
c58ab33 to
eb5eef9
Compare
BenchmarksComparisonBenchmark execution time: 2026-02-26 22:46:37 Comparing candidate commit e90e20c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 57 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
Group 12
Group 13
Group 14
Group 15
Group 16
Group 17
Group 18
Group 19
BaselineOmitted due to size. |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: e90e20c | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1619 +/- ##
=======================================
Coverage 71.21% 71.21%
=======================================
Files 423 424 +1
Lines 62464 62493 +29
=======================================
+ Hits 44481 44504 +23
- Misses 17983 17989 +6
🚀 New features to boost your workflow:
|
Avoid rebuilding TLS configuration for every export by reusing a process-wide cached config in implicit exporter creation paths. This removes repeated certificate-store initialization overhead on most platforms and fixes the CPU regression during profile uploads.
31ed060 to
e90e20c
Compare
gyuheon0h
left a comment
There was a problem hiding this comment.
LGTM. Just a thought; for the use case of a ProfileExporter, if the exporter is always hitting the same endpoint(s) with the same proxy settings, it may be worth caching a reqwest::Client (or a small client pool) that’s built using this TLS config? I'm pretty sure for all the usecases of ProfileExporter. we don't change the config stuff, like headers, timeouts, proxies within the process (plz correct me if I am wrong).
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
9a61cae
into
main
# What does this PR do? Bump version to 28.0.2 This includes: - [chore: update bytes to 1.11.1 to address RUSTSEC-2026-0007](https://github.com/DataDog/libdatadog/pull/1628)[chore: update bytes to 1.11.1 to address RUSTSEC-2026-0007](#1628) - [chore(release): merge release branch to main](https://github.com/DataDog/libdatadog/pull/1629)[chore(release): merge release branch to main](#1629) - [fix: add ecs task metadata to unobfuscated ip addresses](https://github.com/DataDog/libdatadog/pull/1631)[fix: add ecs task metadata to unobfuscated ip addresses](#1631) - [fix: credit card luhn validation for obfuscation](https://github.com/DataDog/libdatadog/pull/1633)[fix: credit card luhn validation for obfuscation](#1633) - [perf(profiling): cache TLS in ProfileExporter::new](https://github.com/DataDog/libdatadog/pull/1619)[perf(profiling): cache TLS in ProfileExporter::new](#1619) # Motivation [perf(profiling): cache TLS in ProfileExporter::new](https://github.com/DataDog/libdatadog/pull/1619)[perf(profiling): cache TLS in ProfileExporter::new](#1619) fixes slow TLS issue # Additional Notes Anything else we should know when reviewing? # How to test the change? Describe here in detail how the change can be validated. Co-authored-by: gyuheon.oh <[email protected]>
| ) -> anyhow::Result<Self> { | ||
| let (builder, request_url) = endpoint.to_reqwest_client_builder()?; | ||
|
|
||
| let tls_config = super::tls::cached_tls_config()?; |
There was a problem hiding this comment.
tls should not be needed for http / uds
There was a problem hiding this comment.
The design of the reqwest library is that a client can connect to multiple URLs. It doesn't know if it will use HTTP/HTTPS until later, but the config is done here.. We can possibly-rewrite the necessary components because we should know if we'll be using HTTPS or not, but that's not how the prior nor current code was designed (probably because we inherit such design unintentionally from tokio/reqwest).
What does this PR do?
Adds a
TlsConfigtype that wraps arustls::ClientConfigpre-configuredwith the platform certificate verifier. This is cached in ProfileExporter::new
with a global variable, similar to what we did in v25.
Motivation
ProfileExporter::new()calledreqwest::ClientBuilder::build(), whichinitialized TLS from scratch every time. On Linux this means loading and
parsing the system certificate store from disk on every exporter creation
— an expensive operation that was identified as a performance regression
when upgrading libdatadog from v25 to v28.
Additional Notes
I dropped an explicit API in favor of an implicit one to get the regression
fixed. We can pursue a newer API later if we want.
The new verifier used in reqwest seems to be more expensive than the TLS verifier we used in v25, but since this cached once per process, this should be okay. However, be aware when benchmarking that it will be a bit more expensive than before, especially for short times like 180 seconds. Here it is in PHP after the fix, left is v28, right is this branch:
How to test the change?
Do everything the same, just notice less overhead because it's not
creating the TLS every time (except on MacOS, which does lazy eval
internally in the library on purpose).