[statsd] Disable statsd buffering by default #692
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
sgnn7
added
resource/dogstatsd
documentation
Due to impact on users of other clients where buffering was enabled by default, especially in environments and frameworks where `fork()` is used we will for the time being disable buffering by default until a decision is made on the path forward. Buffering can still be turned on with `disable_buffering = False` flag it's just that for now it isi defaulted to `True`.
Since we are at least for the time being disabling buffering by default, the docs here are being updated to match the changes applied.
sgnn7
force-pushed
the
sgnn7/temporarly-disable-dsd-buffering
branch
from
c802331
to
a8a12cd
Compare
gh123man
previously approved these changes
Oct 13, 2021
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
With a non-buffered environment, our context manager may not work properly. This change ensures that we propely test this behavior.
gh123man
approved these changes
Oct 13, 2021
truthbk
approved these changes
Oct 13, 2021
| @@ -453,16 +450,16 @@ def open_buffer(self, max_buffer_size=None): | |||
|
|
|||
| self._manual_buffer_lock.acquire() | |||
|
|
|||
| # XXX Remove if `disable_buffering` default is changed to False | |||
| self._send = self._send_to_buffer | |||
There was a problem hiding this comment.
This stuff is not thread-safe. I presume it never really was and I assume that's beyond the expected use-cases.
There was a problem hiding this comment.
@truthbk Generally things should be thread-safe due to various locks on the socket/buffer/context ops but you may get unexpectedly-buffered metrics if some threads are opening/closing buffers while another thread is just writing metrics without a context manager or open_buffer. Overall though, there should be no data loss or exceptions. As for the setter here, the lock + GIL makes this effectively an atomic operation. We do have a test around at least part of this here. If I'm missing something though, I'll fix up the PR for sure.
I presume it never really was and I assume that's beyond the expected use-cases.
Somewhat. The way it was coded originally was not really thread safe but the changes made subsequently over this year have closed up most of the glaring exception issues. The buffering-by-default fixed the self._send = GIL reliance but we're backing that part out with this PR for the time being 😢 .
therve
approved these changes
Oct 14, 2021
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
ewdurbin
added a commit
to pypi/warehouse
that referenced
this pull request
Jan 4, 2022
Since DataDog/datadogpy#692, datadog emits the following logs for every request: ``` pypi-warehouse-web-84cff6b7c7-w7brv web {"logger": "datadog.dogstatsd", "level": "INFO", "event": "Statsd buffering is disabled", "thread": 140486667159360} pypi-warehouse-web-84cff6b7c7-w7brv web {"logger": "datadog.dogstatsd", "level": "INFO", "event": "Statsd periodic buffer flush is disabled", "thread": 140486667159360} ``` This sets the log level to silence these and clean up production logs.
ewdurbin
added a commit
to pypi/warehouse
that referenced
this pull request
Jan 4, 2022
Since DataDog/datadogpy#692, datadog emits the following logs for every request: ``` pypi-warehouse-web-84cff6b7c7-w7brv web {"logger": "datadog.dogstatsd", "level": "INFO", "event": "Statsd buffering is disabled", "thread": 140486667159360} pypi-warehouse-web-84cff6b7c7-w7brv web {"logger": "datadog.dogstatsd", "level": "INFO", "event": "Statsd periodic buffer flush is disabled", "thread": 140486667159360} ``` This sets the log level to silence these and clean up production logs.
domdfcoding
pushed a commit
to domdfcoding/warehouse
that referenced
this pull request
Jun 7, 2022
Since DataDog/datadogpy#692, datadog emits the following logs for every request: ``` pypi-warehouse-web-84cff6b7c7-w7brv web {"logger": "datadog.dogstatsd", "level": "INFO", "event": "Statsd buffering is disabled", "thread": 140486667159360} pypi-warehouse-web-84cff6b7c7-w7brv web {"logger": "datadog.dogstatsd", "level": "INFO", "event": "Statsd periodic buffer flush is disabled", "thread": 140486667159360} ``` This sets the log level to silence these and clean up production logs.
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?
Due to impact on users of other clients where buffering was enabled by
default, especially in environments and frameworks where
fork()isused we will for the time being disable buffering by default until a
decision is made on the path forward. Buffering can still be turned on
with
disable_buffering = Falseflag it's just that for now it isdefaulted to
True.Description of the Change
DogStatsdviastatsdmodule-level object defaults to synchronous (un-buffered) metric sending.DogStatsdvia the default constructor defaults to synchronous (un-buffered) metric sending.Alternate Designs
In discussion (TBD). We may revert this and use automated ways to detect environments
that are incompatible to thread-based buffering.
Possible Drawbacks
Lower performance in high-throughput environments.
Verification Process
Unit tests cover most of this:
python$(python --version 2>&1 | cut -c8-8)" -m unittest -vvv tests.unit.dogstatsd.test_statsdAdditional Notes
This change may be reversed in the future but for now, the risk is too high
for user disruption.
Release Notes
Disable statsd buffering by default. Buffering can be enabled with
disable_buffering = Falseflag.Note: Since this is just a partial revert of #670, removal of both release notes can be done as it's a net-0 change
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.