Change socket_connect_timeout to boolean, include timeout parameter in stop and wait_for_pending, c - #987
Merged
Conversation
_send_to_buffer/_should_flush indexed {False:..., True:...} dicts by
replay_safe, costing a bool() coercion plus a subscript on the single
hottest path in the library (once per metric, ~18k/s per benchmark
client). Measured in the benchmark image: _send_to_buffer 0.6388us on
master -> 0.7344us on this branch (+15.0%), which was ~72% of the whole
per-metric regression (+0.132us, +7.2%).
Hold the two batches as four plain attributes and branch on replay_safe
instead, and inline the size comparison so the hot path makes no call to
_should_flush (which master also paid). _should_flush is retained for the
pre-existing surface and for tests.
Also stop reading the clock in SenderQueue.get() for entries that can
never expire: the argument to _expired() is evaluated before the call, so
every bare-string (replay-safe) get() computed a monotonic() it then
discarded.
Measured after, best-of-7: _send_to_buffer -16.5%, full per-metric -8.5%.
Unit tests: 184 passed, 1 skipped with DD_ORIGIN_DETECTION_ENABLED=false
(the 45 failures otherwise seen are a pre-existing container-id artifact
of the dev host, identical before and after this change).
These four are development aids, not part of the change under review: tests/manual/emulate_reconnect_then_write_fails.py tests/manual/test_sender_queue_manual.py tests/manual/test_shutdown_bound.py tests/performance/test_sender_queue_benchmark.py They are ad-hoc drivers and a benchmark harness rather than tests the suite runs, and together they accounted for 1278 of the ~1970 added lines under tests/, which buried the unit coverage that does matter. Retained locally via .git/info/exclude (which is not committed) so they stay available for development without shipping in the review.
tests/manual/ holds ad-hoc development drivers, not tests the suite runs, so none of it belongs in the review. This removes the one remaining tracked file, test_gauge_with_timestamp_aggregation.py, leaving tests/manual/ entirely untracked. Retained on disk via .git/info/exclude, which now excludes the whole directory so future scratch scripts there cannot be added by accident.
6 tasks
atanzu
reviewed
Sep 18, 2026
| return True | ||
|
|
||
| # Condition.wait()'s return value can't be used to detect a | ||
| # timeout: on Python 2 it is always None. Track the deadline |
There was a problem hiding this comment.
I guess we can leave this one out of scope, python 2 is EOL anyway.
Contributor
Author
There was a problem hiding this comment.
Not for datadogpy!
atanzu
reviewed
Sep 18, 2026
| sender_queue_timeout=0, # type: Optional[float] | ||
| track_instance=True, # type: bool | ||
| socket_connect_timeout=DEFAULT_SOCKET_CONNECT_TIMEOUT, # type: Optional[float] | ||
| socket_connect_retry=False, # type: bool |
There was a problem hiding this comment.
Shall we note that in some readme or release notes?
atanzu
reviewed
Sep 18, 2026
|
|
||
| backoff = UDS_CONNECT_RETRY_INITIAL_BACKOFF | ||
|
|
||
| while deadline is None or time.time() < deadline: |
Contributor
Author
There was a problem hiding this comment.
It essentially falls back on the main loop here.
# Conflicts: # datadog/dogstatsd/base.py # tests/unit/dogstatsd/test_statsd.py
# Conflicts: # datadog/dogstatsd/base.py # datadog/dogstatsd/sender_queue.py # tests/unit/dogstatsd/test_statsd.py
StephenWakely
marked this pull request as ready for review
September 22, 2026 09:53
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfa9fb0bfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
StephenWakely
added
the
changelog/Changed
MintsInc
approved these changes
Sep 22, 2026
carlosroman
approved these changes
Sep 22, 2026
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Requirements for Contributing to this repository
What does this PR do?
Follow on from #986
socket_connect_timeoutconfiguration option to a booleansocket_connect_retrystopandwait_for_pendingfunctions.Description of the Change
With #986 the sender queue effectively drops older metrics from the queue when it fills up. With that in place, when the client is unable to connect to the agent we want to continue to attempt connection for as long as possible. Dropping a metric is not determined by an arbitrarily configured timeout, instead it is down to the queue to determine when a metric gets dropped. The configured queue size determines how much memory the client is willing to dedicate to holding on to unsent metrics.
The
stopandwait_for_pendingfunctions need a timeout parameter to determine how long we should wait given the possibility that we may never be able to clear the queue if the connection cannot be created.Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.