You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Bound DogStatsD stop() final flush by remaining timeout - #1012
PR #1009 made stop() report failures from the final synchronous flush. However, after the background sender consumes part or all of stop(timeout), that final flush was still bounded by the configured socket_timeout, so the total wall-clock bound could become timeout + socket_timeout.
This follow-up clamps the main and telemetry socket timeouts to the remaining stop(timeout) budget during the final synchronous flush, then restores the configured socket timeouts before closing sockets. If the stop budget is already exhausted, the final flush uses a zero socket timeout and becomes a non-blocking attempt.
Changelog
[Fixed] Bound DogStatsD stop(timeout) final synchronous flush by the remaining timeout budget.
stop(timeout) previously handed the final synchronous flush a fresh socket_timeout, so the overall bound became timeout + socket_timeout (and worse with several packets). Record a per-thread deadline for that flush and clamp socket_timeout to the budget left before each synchronous send, skipping further sends once it is gone.
The deadline is thread-local and the clamp is undone before _socket_lock is released, so a send racing the flush from another thread keeps the configured socket_timeout instead of inheriting the clamp or being dropped once the stop budget expires. stop() no longer snapshots and restores socket_timeout either, so it cannot revert a set_socket_timeout() that raced the flush.
Thread the stop() final-flush deadline through the call chain
(_flush_aggregated_metrics/_flush_buffered_metrics -> _flush_one_buffer
-> _send_to_server -> _xmit_packet_with_telemetry -> _xmit_packet ->
_xmit_packet_attempt -> _clamp_send_timeout) as an explicit parameter
instead of threading.local(). Each thread's call stack already scopes
the value correctly on its own, so there is no shared/ambient state to
leak a deadline onto an unrelated concurrent send, and no state to
reset after a fork.
Also fixes a latent bug this uncovered: stop() was calling the public
flush_aggregated_metrics()/flush_buffered_metrics() wrappers (which
never receive a deadline), so the explicit deadline never reached
_clamp_send_timeout() through those two methods specifically. Routing
stop() through the private _flush_aggregated_metrics(deadline) and
_flush_buffered_metrics(deadline) variants surfaced that
_flush_buffered_metrics's existing 'skip the second buffer once the
deadline is gone' short-circuit needs its 'complete' return value
combined with 'sent' for stop()'s purposes -- a skipped buffer still
means stop() did not actually deliver everything.
Replace _clamp_send_timeout() (which shrank socket_timeout to whatever
was left of stop(timeout)'s budget, then restored it) with
_within_send_budget(), which only checks whether the deadline has
already passed before attempting a *new* send.
A send already underway now always gets the full configured
socket_timeout -- it is never shrunk -- so stop(timeout)'s resolution
becomes one socket_timeout rather than an arbitrary fraction of one.
This also removes the need to snapshot/restore socket_timeout around
each send, and the _NO_CLAMP sentinel that tracked whether a restore
was pending.
Updates test_stop_final_flush_bounds_each_send_by_the_remaining_timeout
and test_stop_final_flush_leaves_other_threads_sends_unclamped (renamed
to reflect the new behavior) to assert on whether a send was attempted
at all, rather than on the socket_timeout value is was attempted with
-- the latter is no longer touched by any send, clamped or not.
Remove the deadline parameter from the whole _report/_send/
_send_to_buffer/_send_to_server/_xmit_packet_with_telemetry/
_xmit_packet/_xmit_packet_attempt chain and _within_send_budget()
entirely. The deadline is now checked only where a flush already
loops over multiple items:
- _flush_buffered_metrics() already checked it between its two
_flush_one_buffer() calls; that call no longer threads deadline any
deeper.
- _flush_aggregated_metrics() gets the same treatment, newly: checked
once per metric, before attempting it.
Tradeoff accepted: a send already underway always runs to completion
(same resolution-of-one-socket_timeout design as before), but a
telemetry flush piggybacked inside _xmit_packet_with_telemetry can no
longer be skipped independently of the main packet it rides with --
only the next *item* in a flush loop is skipped, not a second send
nested inside one already in flight. In exchange, deadline no longer
needs threading through 7 call signatures.
aggregator.flush_aggregated_metrics()/flush_aggregated_sampled_metrics()
drain their metrics out of the aggregator entirely, unlike a skipped
buffered line (which just stays in self._buffer_ts) -- so an abandoned
metric has nowhere left to sit. Account it in packets_dropped_writer
instead, or it vanishes with no trace. Added
test_stop_final_flush_accounts_aggregated_metrics_abandoned_by_the_deadline
to cover this.
Reverted the now-vestigial deadline=None parameter from test doubles
that stub _xmit_packet_with_telemetry/_send_to_server, and fixed two
_flush_one_buffer wrappers that forwarded deadline= as a keyword
(which _flush_one_buffer no longer accepts).
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Final flushing can exceed stop(timeout) because main and telemetry socket operations lack remaining-budget limits, and buffered sends can start after the deadline while still reporting success.
This follow-up to #1009 adds deadline-aware final flushing to DogStatsD shutdown, aiming to limit work after the stop(timeout) budget expires while preserving failure reporting.
Changes:
Passes a stop deadline into aggregated and buffered flushes.
Counts aggregated metrics abandoned after expiry as writer drops.
Adds regression tests for deadline handling, concurrent sends, and socket-timeout updates.
File
Description
tests/unit/dogstatsd/test_statsd.py
Adds shutdown deadline and concurrency regression tests.
datadog/dogstatsd/base.py
Adds final-flush deadline checks and abandoned-metric accounting.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Shutdown socket operations can exceed the requested timeout or block indefinitely, while exhausted-budget handling skips non-blocking sends and discards aggregates without fully accounting for dropped bytes.
When the deadline expires, this path discards the remaining aggregates but increments only packets_dropped_writer, leaving bytes_dropped_writer and total dropped-byte telemetry understated. Account for their serialized payload sizes as well as their packet count, using the reporting path's normalization and newline framing. The same issue occurs in the sampled-metric exit at line 1388.
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
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.
StephenWakely
added
resource/dogstatsd
changelog/Fixed
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Summary
Depends on #1009.
PR #1009 made
stop()report failures from the final synchronous flush. However, after the background sender consumes part or all ofstop(timeout), that final flush was still bounded by the configuredsocket_timeout, so the total wall-clock bound could becometimeout + socket_timeout.This follow-up clamps the main and telemetry socket timeouts to the remaining
stop(timeout)budget during the final synchronous flush, then restores the configured socket timeouts before closing sockets. If the stop budget is already exhausted, the final flush uses a zero socket timeout and becomes a non-blocking attempt.Changelog
stop(timeout)final synchronous flush by the remaining timeout budget.Testing
flake8 datadogmypy datadog/dogstatsd/base.py datadog/dogstatsd/sender_queue.pyDD_ORIGIN_DETECTION_ENABLED=false pytest -q tests/unit/dogstatsd— 232 passed, 1 skipped