ekump/APMSP-1805 lazily init cadence client #890
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
ekump
force-pushed
the
ekump/APMSP-1805-lazily-init-cadence-client
branch
from
7dbf470 to
0d252c6
Compare
ekump
commented
Feb 19, 2025
| use std::thread; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_thread_safety() { |
There was a problem hiding this comment.
I'm not sure how useful this test is? It is just checking that we don't set a client until after we send. I couldn't think of a better way to unit test this.
ekump
force-pushed
the
ekump/APMSP-1805-lazily-init-cadence-client
branch
from
0d252c6 to
6714915
Compare
ekump
force-pushed
the
ekump/APMSP-1805-lazily-init-cadence-client
branch
from
6714915 to
0f7acb1
Compare
BenchmarksComparisonBenchmark execution time: 2025-02-20 23:14:59 Comparing candidate commit 13124e6 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 51 metrics, 2 unstable metrics. scenario:benching deserializing traces from msgpack to their internal representation
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
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #890 +/- ##
==========================================
+ Coverage 71.75% 71.79% +0.03%
==========================================
Files 328 328
Lines 48514 48577 +63
==========================================
+ Hits 34813 34874 +61
- Misses 13701 13703 +2
|
ekump
commented
Feb 19, 2025
| /// Set the destination for dogstatsd metrics, if an API Key is provided the client is disabled | ||
| /// as dogstatsd is not allowed in agentless mode. Returns an error if the provided endpoint | ||
| /// is invalid. | ||
| pub fn set_endpoint(&mut self, endpoint: Endpoint) -> anyhow::Result<()> { |
There was a problem hiding this comment.
This function wasn't used anywhere else in libdatadog. I can leave it in as it is technically a breaking API change if we think the crate is being used outside of libdatadog.
bantonsson
reviewed
Feb 20, 2025
bantonsson
reviewed
Feb 20, 2025
ekump
force-pushed
the
ekump/APMSP-1805-lazily-init-cadence-client
branch
from
4933211 to
c5bb7c9
Compare
APMSP-1805. Sidecar potentially has 100s of sessions, each of which creates dogstatsd clients. Defer initalizing new cadence clients until we are actually ready to send. Cadence creates a new OS thread every time a client is initialized.
ekump
force-pushed
the
ekump/APMSP-1805-lazily-init-cadence-client
branch
from
c5bb7c9 to
61d2838
Compare
ekump
force-pushed
the
ekump/APMSP-1805-lazily-init-cadence-client
branch
from
9d3ee1a to
b37dc4e
Compare
ekump
force-pushed
the
ekump/APMSP-1805-lazily-init-cadence-client
branch
from
b37dc4e to
13124e6
Compare
bantonsson
approved these changes
Feb 21, 2025
| DogStatsDAction::Set(metric, value, tags) => { | ||
| do_send(client.set_with_tags(metric.as_ref(), value), tags) | ||
| let client_opt = match self.get_or_init_client() { | ||
| Ok(guard) => guard, |
There was a problem hiding this comment.
Nitpick: variable name guard is a bit confusing since it's the client option now.
There was a problem hiding this comment.
ah yes, forgot to update this variable. I'll make a note to update it in another PR.
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?
Wait until send is called on dogstatsd client to initialize the cadence client.
Motivation
After this PR PHP customers in serverless environments started to experience an issue running out of file descriptors / threads. It is believed to be related to the creation of a new cadence client when initializing a new dogstatsd_client. This PR should re-establish previous behavior where the cadence client is lazily initialized when sending stats.
Additional Notes
We couldn't simply revert to using the previous code in the sidecar that lazily initialized the cadence client because the dogstatsd client is used in the Data Pipeline Trace Exporter inside async code and you can't just make the
sendmethod&mut selfto allow us to set the client. So we have to use interior mutability viaArcandMutex.How to test the change?
Unit tests added and existing ones continue to work. PHP team manually verified this resolves the thread count issue.