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

Flush metrics with BufferedUdpMetricSink #96

Closed
parasyte opened this issue Jan 27, 2020 · 2 comments
Closed

Flush metrics with BufferedUdpMetricSink #96

parasyte opened this issue Jan 27, 2020 · 2 comments

Comments

@parasyte
Copy link

In environments where metrics are constantly emitted, there is no need to flush the buffer. But in situations where the environment is only sparsely used, metrics can sit in the buffer for arbitrarily long durations.

A similar (but potentially more pressing) issue is that BufferedUdpMetricSink has no way to flush the contents of its buffer when dropped, making data loss inevitable.

@56quarters
Copy link
Owner

56quarters commented Jan 27, 2020

But in situations where the environment is only sparsely used, metrics can sit in the buffer for arbitrarily long durations.

Yes, it's possible for metrics to sit in the buffer for a while. I can't think of a good way around this that doesn't involve more complexity than the sink itself. In this case, I think the prudent thing is for users to use the UdpMetricSink instead since they presumably don't have a lot of metrics being emitted to necessitate buffering. Where do you think a good place to document this tradeoff is? Perhaps in the docs for each of the sinks? Or the main lib docs (lib.rs + README)?

A similar (but potentially more pressing) issue is that BufferedUdpMetricSink has no way to flush the contents of its buffer when dropped, making data loss inevitable.

The BufferedUdpMetricSink doesn't flush on drop explicitly but the underlying BufWriter that our write adapter uses, does: https://github.com/tshlabs/cadence/blob/4edb69ca281dfbb4651d26d4266cc6c4307c5943/src/io.rs#L215

@parasyte
Copy link
Author

Glad to see the writer will flush itself.

I suppose the environmental edge case can be handled with a configuration flag, so this isn't a showstopper either. If anything, I would probably document this behavior on BufferedUdpMetricSink, and maybe mention it in passing on the README where this type is introduced. I think that would sufficiently resolve this ticket!

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

No branches or pull requests

2 participants