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

enable the ability for buffering and aggregation to work at the same #851

Merged

Conversation

andrewqian2001datadog
Copy link
Contributor

@andrewqian2001datadog andrewqian2001datadog commented Aug 21, 2024

What does this PR do?

Issue

enable the ability for buffering and aggregation to work at the same

Description of the Change

Refactors code to enable aggregation/buffering at the same time. The aggregator will add the aggregated metrics into the buffer at the end of every flush interval before the buffer sends them to the agent. Aggregation/buffering share the same flush thread and flush interval.

Possible Drawbacks

aggregation and buffering seems tightly coupled, for example

Instead of killing the flush thread when buffering is disabled, (I assume) it needs to check if both aggregation and buffering is disabled before killing the flush thread. May be a good idea to add a if statement in stop_flush_thread to not kill the flush thread if either aggregation or buffering is enabled?

Verification Process

Added unit tests to test aggregation and buffering at the same time.

For Local testing:
Follow these steps for local testing and in the main.py file, enable aggregation and buffering at the same time

main.py

from datadog import initialize, statsd
import time

options = {
    "statsd_host": "127.0.0.1",
    "statsd_port": 8125,
    "statsd_disable_buffering" : False,
    "statsd_disable_aggregation" : False,
    "statsd_aggregation_flush_interval" : 10
}

initialize(**options)

while(1):
  print("-------------------------------------------------")
  print("metric added")
  statsd.count('testandrew123.count', 1, tags=["environment:dev"], sample_rate=1)
  time.sleep(1)

You can add a print statement here to ensure that the agggregated metrics are added to buffer, we would expect that the count metrics are aggregated (single metric with value of 10) inside the buffer

Additional Notes

Release Notes

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.

@andrewqian2001datadog andrewqian2001datadog self-assigned this Aug 21, 2024
@andrewqian2001datadog andrewqian2001datadog marked this pull request as ready for review August 22, 2024 18:35
@andrewqian2001datadog andrewqian2001datadog added the changelog/Added Added features results into a minor version bump label Aug 22, 2024
gh123man
gh123man previously approved these changes Aug 22, 2024
Copy link
Member

@gh123man gh123man left a comment

Choose a reason for hiding this comment

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

Code changes look good to me. Just a minor suggestion to align the config naming.
Lets be sure we manually test/verify:

  • aggregation + buffering
  • buffering only
  • aggregation only
    (just for completeness).

datadog/dogstatsd/base.py Outdated Show resolved Hide resolved
datadog/__init__.py Outdated Show resolved Hide resolved
@rayz
Copy link

rayz commented Aug 22, 2024

Would there be any potential problems with having the default aggregation flush interval that low? old 2s -> new .3s

@andrewqian2001datadog
Copy link
Contributor Author

Not sure but per vikentiy "I don't have an opinion whether they should share a thread, whatever is simpler I guess. We probably should not change the default flush interval though." @rayz

@andrewqian2001datadog andrewqian2001datadog merged commit 7b721e3 into master Aug 23, 2024
11 checks passed
@andrewqian2001datadog andrewqian2001datadog deleted the enable-buffering-aggregation-simultaneously branch August 23, 2024 13:57
@gorangasic
Copy link

@gh123man @carlosroman would it be possible to cut a v0.50.1 release with this PR included?

@carlosroman
Copy link
Contributor

@gorangasic have created the release PR #856. Hopefully should all get approved really soon.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants