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] Add ability to toggle statsd.disable_buffering state during runtime #700

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

sgnn7
Copy link
Contributor

@sgnn7 sgnn7 commented Nov 10, 2021

What does this PR do?

  • Adds ability to set statsd_instance.disable_buffering flag after initialization
  • Adds said flag as a argument to initialize() to be usable for the module-level statsd instance

Description of the Change

While not likely to be useful currently, if/when we enable buffering on the DogStatsd clients
by default again we will want to have this feature already in place since the module level statsd
may not behave as expected in fork()-based environments. Because datadog.initialize() cannot
recreate the instance of the module-level statsd, we need a runtime setting for disable_buffering
instead of recreating the instance, making the implementation somewhat constrained in how it is
done.

As a side-effect, this change allow use of the module-level statsd instance with buffering enabled
which was previously not possible.

Alternate Designs

None that would make sense with the limitations provided.

Possible Drawbacks

Assumptions were made in the logic that datadog.initialize() would be only called once for
the application. While there are protections around concurrent invocations, there could be edge
cases not covered if multiple concurrent calls to `initialize are made to change this flag or if the
data is being added to the buffer while the buffering is being disabled.

Verification Process

Tests covers the underlying functionality of this feature but it can be tested with:

  • Using either:
    • datadog.initialize() and setting statsd_disable_buffering flag
    • Instantiating DogStatsd
  • Verifying that data flows through

Additional Notes

This feature will be gradually documented as it is expected to be
useful only in very niche high-throughput cases.

Release Notes

  • [statsd] Add ability to toggle statsd.disable_buffering state during runtime
  • [datadog] Add ability to use statsd_disable_buffering as an argument to datadog.initialize()

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.

Since `initialize` uses the module-level DogStatsd instance that is only
initialized once, we cannot rely on new instantiation for changes to the
`disable_buffering` flag. This change allows the consumers to call
`stats.disable_buffering = <bool>` with runtime changes being applied to
the module-level instance. By doing this, we can in future changes allow
`initialize()` to change this flag if meeded.
Now that the DogStatsd module can handle live toggling of buffering
switches, we expose this argument in the `datadog.initialize()`. While
not terribly useful currently, it will be very valuable if/when
buffering is enabled on the DogStatsd module by default (again).
@sgnn7 sgnn7 added resource/dogstatsd changelog/Added Added features results into a minor version bump kind/feature-request Feature request related issue severity/normal Normal severity issue labels Nov 10, 2021
@sgnn7 sgnn7 added this to the 1.0.0 milestone Nov 10, 2021
@sgnn7 sgnn7 self-assigned this Nov 10, 2021
@sgnn7 sgnn7 requested review from a team as code owners November 10, 2021 23:01
@therve
Copy link
Contributor

therve commented Nov 11, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

I don't think I have enough context to approve this, but it looks good to me as far as I understand. I'll re-request agent-core review.

@djmitche djmitche requested a review from a team November 11, 2021 19:06
@@ -37,6 +37,7 @@ def initialize(
api_host=None, # type: Optional[str]
statsd_host=None, # type: Optional[str]
statsd_port=None, # type: Optional[int]
statsd_disable_buffering=True, # type: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it named disable_buffering because the plan is to have the buffering enabled by default in the future? If that's the case, don't mind the following remark.
Having a parameter "disabling" something (and defaulting to True) is always weird to think about opposed to a parameter "enabling" a feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it named disable_buffering because the plan is to have the buffering enabled by default in the future

Correct. The long story is that it's currently a bit of an awkward flag name like you mention because we enabled-then-disabled buffering-by-default already and the flag that was already in the API was disable_buffering. If/when we re-enable buffering-by-default, this flag will be a better indication of the functionality it's just that for now we're a bit stuck with a mid-way change in architectural direction and the flag was already named in the published API.


self._flush_thread_stop.set()

self._flush_thread.join()
Copy link
Contributor

@remeh remeh Nov 16, 2021

Choose a reason for hiding this comment

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

Not really a big deal I guess but if two threads are calling _stop_flush_thread simultaneously, there is an small chance that join() could be called on one of the thread while the _flush_thread has already be terminated by the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True that this method isn't thread-safe in isolation but the comment on the method mentions this as a warning and the only caller is lock-protected. I would have preferred to use the lock directly in this method like you imply but then the toggle to stop would needed 2 lock acquires (1 for toggle method and 1 for stop method; possibly a reentrant one) which seemed like an overkill and over-complication for a helper only called from a single spot.

Copy link
Contributor

@remeh remeh left a comment

Choose a reason for hiding this comment

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

I don't know the codebase extremely well but changes in this PR looks good to me!

@sgnn7 sgnn7 merged commit 6bf4578 into master Nov 17, 2021
@sgnn7 sgnn7 deleted the sgnn7/add-buffering-toggle branch November 17, 2021 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump 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