-
Notifications
You must be signed in to change notification settings - Fork 11
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
Support custom sinks #23
Conversation
Linked cadence issue from #22 mentions that they want to flush metrics during a quiet period. I guess the goal with such a feature would be that you get the metrics reported consistently vs relying on shutdown when your buffer is not full. I do think that we had seen a drop method implemented somewhere in cadence that does flush before shutting down, so I'm not sure if more is needed to be done but we always validate that it's doing what 'it's supposed to. |
In Destructors are not called for global
|
/// # Ok(()) | ||
/// # } | ||
/// ``` | ||
pub fn with_sink<T>(mut self, sink: T) -> Self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently overriding the other sink configs is a pretty big surprise. Maybe this could be implemented here in a way that bypasses the builder type?
For example: fn StatsdBuilder::from_sink<T>(sink: T) -> StatsdRecorder
That being said, I'm not sad about the public StatsdRecorder
usage either:
let sink = build_sink();
let statsd_client = StatsdClient::from_sink("prefix", sink);
let recorder = StatsdRecorder::new(statsd_client, HistogramType::Histogram);
metrics::set_boxed_recorder(Box::new(recorder))
.expect("metrics recorder should be uninitialized");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a little sheepish about this, but I sketched it out in the way you suggest, and I think this is probably the least awkward way. In any case we can improve on it later. Thanks for your patience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a method to the builder that allows users to specify their own
MetricSink
instead of unconditionally using UDP.This is an alternate way to do what #22 is doing, and make the functionality a little more discoverable maybe. It's honestly not a ton better. Awkward -- the "builder pattern" is clunky for what we're doing.
Includes docs and tests, though.
Edit: Let's not land this until we decide what to do in #22. It would be awfully nice to support "don't lose metrics on shutdown" out of the box.