When I saw we had two config options, ttl_days and ttl, it made sense to pick one (as we did recently by removing ttl_days in #33648)
Unfortunately when I approved this, I didn't realize that the largest unit of time for time.Duration is hours (h)
For telemetry use cases (mainly logging), data is kept for much longer than most people measure in hours. For example, I know a day is 24h, but what if I want to keep my log data for 6 months? While configuring my exporter I thought I could simply do 180d, but this of course fails to parse. It's not that doing the calculation to write 4320h is hard, it's just incredibly unintuitive. It's an easy mistake to make to type d there, especially for those who don't know Go and the time package.
I believe it would be convenient to expand this to at least include days (d), but if we were to do this, where would be the best place?
Here are the different options, in order of least complexity:
re-add ttl_days. We can still mark it as deprecated, but it is very convenient.
In clickhouseexporter: we can change ttl from time.Duration to string and parse it in the local config parser function. We would add some logic for when the time contains d, but otherwise fallback to to time.ParseDuration.
In OTel confmap: We can modify the hook here, so that all other components and configs can benefit from the change. We would have to write our own hook with the same change as option 1.
In go-viper: This is the top-most parser we could modify without modifying Go itself. time.Duration does not have Unmarshal* functions declared, so this module adds a hook to include it. We could add a new function called StringToTimeDurationExtendedHookFunc to expand the current behavior, and then use it in OTel confmap.
This would be a non-breaking change since it doesn't affect any of the existing units. This change could benefit all OTel configs, not just ClickHouse exporter. Let me know what you think.
Thanks!
Note in case anyone points it out: This convenience is for users on Earth, and the conventional understanding that a day is 24 hours, rounded for convenience.
Component(s)
exporter/clickhouse
Describe the issue you're reporting
When I saw we had two config options,
ttl_daysandttl, it made sense to pick one (as we did recently by removingttl_daysin #33648)Unfortunately when I approved this, I didn't realize that the largest unit of time for
time.Durationis hours (h)For telemetry use cases (mainly logging), data is kept for much longer than most people measure in hours. For example, I know a day is
24h, but what if I want to keep my log data for 6 months? While configuring my exporter I thought I could simply do180d, but this of course fails to parse. It's not that doing the calculation to write4320his hard, it's just incredibly unintuitive. It's an easy mistake to make to typedthere, especially for those who don't know Go and thetimepackage.The majority of units in
time.ParseDurationare too small to be usable for a Database TTL config.I believe it would be convenient to expand this to at least include days (
d), but if we were to do this, where would be the best place?Here are the different options, in order of least complexity:
ttl_days. We can still mark it as deprecated, but it is very convenient.clickhouseexporter: we can changettlfromtime.Durationtostringand parse it in the local config parser function. We would add some logic for when the time containsd, but otherwise fallback to totime.ParseDuration.time.Durationdoes not haveUnmarshal*functions declared, so this module adds a hook to include it. We could add a new function calledStringToTimeDurationExtendedHookFuncto expand the current behavior, and then use it in OTel confmap.This would be a non-breaking change since it doesn't affect any of the existing units. This change could benefit all OTel configs, not just ClickHouse exporter. Let me know what you think.
Thanks!
Note in case anyone points it out: This convenience is for users on Earth, and the conventional understanding that a day is 24 hours, rounded for convenience.