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

StatsdClient flush functionality #200

Merged
merged 11 commits into from
Mar 20, 2024
Merged

Conversation

James-Bartman
Copy link
Contributor

Problem

The StatsDClient utilizes a MetricSink, which can optionally be buffered. Buffering helps save network bandwidth; however, the BufferedUdpMetricSink won't be flushed until it is full. For, an application which emits relatively few time-sensitive metrics in conjunction with a large amount of time-insensitive metrics, the default buffer flushing may not give the user enough control and result in faulty metric collection (especially since StatsD does not tag metics with a timestamp).

Solution

I have added a pub fn flush() to the StatsDClient which gives users the control they need to flush time-sensitive metrics while also allowing the regular buffer functionality to flush once the buffer is full. Also, I added a ToCounterValue implementation for the u64 type since most counters are unsigned.

@56quarters
Copy link
Owner

56quarters commented Mar 7, 2024

Hi! Thanks for the PR. Adding a flush() method to StatsdClient was previously proposed in #176. I'm still not a huge fan of adding that method and would rather have a sink that periodically flushes (every 1s or so) but it seems like people want this functionality.

I'm curious about your opinion @mlowicki since you recently made some flush() related changes to the QueuingMetricSink.

Also, I added a ToCounterValue implementation for the u64 type since most counters are unsigned.

Do you mind opening a separate PR for those changes? Thank you!

@James-Bartman
Copy link
Contributor Author

Yeah we didn't see any sinks that flush on a time interval and also figured some manual control would be best for developers who need it. And sure, I will separate out the ToCounterValue changes. Thanks!

@56quarters 56quarters changed the title StatsDClient flush functionality and ToCounterValue for u64 StatsdClient flush functionality Mar 9, 2024
@mlowicki
Copy link
Contributor

Hi! Thanks for the PR. Adding a flush() method to StatsdClient was previously proposed in #176. I'm still not a huge fan of adding that method and would rather have a sink that periodically flushes (every 1s or so) but it seems like people want this functionality.

I'm curious about your opinion @mlowicki since you recently made some flush() related changes to the QueuingMetricSink.

It should be still possible to manually flush by keeping reference to the underlying sink and calling flush() directly on such sink so method on StatsdClient seems redundant (although it may simplify the code by avoiding a bit of code to wrap the sink with Arc and so on).

Copy link
Owner

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

OK, I will defer to users instead of forcing everyone to copy the code from examples/wrapped.rs

cadence/src/client.rs Outdated Show resolved Hide resolved
James-Bartman and others added 3 commits March 19, 2024 09:50
Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>
@James-Bartman
Copy link
Contributor Author

Sounds good, I've made the changes and everything should be RTG.

Copy link
Owner

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

One last change and I think this is good to merge.

cadence/src/client.rs Outdated Show resolved Hide resolved
cadence/src/client.rs Outdated Show resolved Hide resolved
Copy link
Owner

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

Thanks!

@56quarters 56quarters merged commit d465482 into 56quarters:master Mar 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants