ref: use Duration for flush_interval #55
Conversation
This PR uses a fixed commit for `statsdproxy` that takes a `Duration` instead of u64 seconds as `flush_interval` so we can see if reducing the `flush_interval` helps with the metrics problem. This is meant as a proof of concept, if it works we will update the `statsdproxy` crate to support it regulary. statsdproxy PR: getsentry/statsdproxy#55
| pub aggregate_gauges: bool, | ||
| #[cfg_attr(feature = "cli", serde(default = "default_flush_interval"))] | ||
| pub flush_interval: u64, | ||
| pub flush_interval: Duration, |
There was a problem hiding this comment.
Should this have a custom deserializer so we can still parse e.g. 1 into seconds?
There was a problem hiding this comment.
Yes that makes a lot of sense. I added the custom deserializer which defaults plain numbers to seconds (for backwards compatibility) while parsing numbers with unit suffixes using https://docs.rs/humantime/latest/humantime/index.html
There was a problem hiding this comment.
I think humantime is too complicated here. why do we want it? does it just happen to align with how Relay represents duration in its config format? we use plain numbers in snuba, for example.
There was a problem hiding this comment.
The idea is that we have backwards compatibility by assuming that plain numbers are seconds while providing a possibility to have sub-second intervals.
I added humantime because I didn't want to roll time string parsing myself, but I can also simplify it by allowing seconds or milliseconds only and write the parsing code myself, what do you thing?
There was a problem hiding this comment.
Relay configures this only in code, so the main concern was backwards compatibility
There was a problem hiding this comment.
I guess we could to get rid of the units entirely, although I'm not too happy to use floats when it's possible to represent it with integers
There was a problem hiding this comment.
We don't use "humantime" for any configuration parameters at all, in any YAML file in {snuba, sentry}. rather we use a numeric value and specify the unit as part of the name of the configuration parameter, such as flush_interval_ms: 1000.
I think preserving backwards compat can be done purely with serde primitives and custom deserializers + alias, but I would also be OK with just making that breaking change. Snuba does not make this kind of thing configurable in the first place, so there are no config files for us to migrate. I don't know about Relay.
There was a problem hiding this comment.
so can we just make this a float then? instead of pulling in a new dep
Fair point with the new dep, I would still suggest to allow plain numbers (= seconds) or using the ms suffix to treat it as milliseconds. This should give us enough flexibility without having more dependencies while keeping the parsing code fairly easy. What do you think?
There was a problem hiding this comment.
We don't use "humantime" for any configuration parameters at all, in any YAML file in {snuba, sentry}. rather we use a numeric value and specify the unit as part of the name of the configuration parameter, such as
flush_interval_ms: 1000.I think preserving backwards compat can be done purely with serde primitives and custom deserializers +
alias, but I would also be OK with just making that breaking change. Snuba does not make this kind of thing configurable in the first place, so there are no config files for us to migrate. I don't know about Relay.
Thanks for the write up, I will look into adding support for an additional parameter that represents ms.
There was a problem hiding this comment.
After giving it a bit more thought I decided to introduce the breaking change for the sake of simplicity
|
@Litarnus what's the status on this? Relay is still on this fork/branch. |
This PR changes the
flush_intervaltoDurationin order to allow sub-second flushing