Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[statsd] Disable statsd buffering by default #692

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

sgnn7
Copy link
Contributor

@sgnn7 sgnn7 commented Oct 13, 2021

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() 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 is
defaulted to True.

Description of the Change

  • Use of DogStatsd via statsd module-level object defaults to synchronous (un-buffered) metric sending.
  • Use of DogStatsd via 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_statsd

Additional 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 = False flag.

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)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@sgnn7 sgnn7 added resource/dogstatsd documentation Documentation related changes changelog/Changed Changed features results into a major version bump kind/feature-request Feature request related issue severity/normal Normal severity issue labels Oct 13, 2021
@sgnn7 sgnn7 added this to the Next milestone Oct 13, 2021
@sgnn7 sgnn7 requested review from a team as code owners October 13, 2021 18:58
@sgnn7 sgnn7 self-assigned this Oct 13, 2021
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 sgnn7 force-pushed the sgnn7/temporarly-disable-dsd-buffering branch from c802331 to a8a12cd Compare October 13, 2021 19:05
gh123man
gh123man previously approved these changes Oct 13, 2021
@sgnn7
Copy link
Contributor Author

sgnn7 commented Oct 13, 2021

/azp run

@azure-pipelines
Copy link

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.
Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I added a note, but if the answer dismisses any concerns I'm OK with the change.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stuff is not thread-safe. I presume it never really was and I assume that's beyond the expected use-cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

therve commented Oct 14, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@therve therve merged commit 2cba9f6 into master Oct 14, 2021
@therve therve deleted the sgnn7/temporarly-disable-dsd-buffering branch October 14, 2021 12:35
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
Labels
changelog/Changed Changed features results into a major version bump documentation Documentation related changes kind/feature-request Feature request related issue resource/dogstatsd severity/normal Normal severity issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants