feat(outputs.redistimeseries): Add option to expire values#18337
feat(outputs.redistimeseries): Add option to expire values#18337skartikey merged 5 commits intoinfluxdata:masterfrom
Conversation
|
Thanks so much for the pull request! |
|
!signed-cla |
trgwii
force-pushed
the
outputs_redistimeseries_expire
branch
from
771ae54 to
09182ea
Compare
trgwii
changed the title
telegraf-tiger
bot
added
feat
trgwii
force-pushed
the
outputs_redistimeseries_expire
branch
from
09182ea to
a94ca4b
Compare
trgwii
force-pushed
the
outputs_redistimeseries_expire
branch
from
a94ca4b to
134d6b6
Compare
mstrandboge
left a comment
mstrandboge
left a comment
There was a problem hiding this comment.
Thanks for the PR! I have a couple minor comments
trgwii
force-pushed
the
outputs_redistimeseries_expire
branch
from
d0965d8 to
061f31f
Compare
|
Looks like all CI runs are failing the |
Yes, this is unrelated to your PR so it won't affect merging |
mstrandboge
added
the
ready for final review
skartikey
left a comment
skartikey
left a comment
There was a problem hiding this comment.
The PR adds a feature but no tests. At minimum, a new testcase directory (e.g., testcases/expire/) should be added to exercise the expire path. The existing TestCases framework already supports loading configs from directories, so adding a test config with expire = "60s" and verifying the key has a TTL set would be straightforward.
- Add a testcase for expiry, modify redistimeseries_test.go to append "; expires" to the end of each line with a TTL set - Switch from always using a pipeline to only doing it when expire is enabled - Update sample configuration to reflect the default of no expiry and clarify the description
I have created a testcase, and also slightly modified redistimeseries_test.go. Please let me know if you would like this implemented in a different way, but this seemed like the most obvious way to communicate whether or not a specific key has TTL set without introducing flakiness like matching on "how many seconds until expiry". |
|
The CI managed to pass redistimeseries before crashing! |
skartikey
left a comment
skartikey
left a comment
There was a problem hiding this comment.
@trgwii Thanks for the PR! A couple of comments for you to take a look at.
No validation for Expire value
File: redistimeseries.go (no Init() method)
A zero duration would cause Redis to delete the key immediately after writing, effectively making the plugin useless. A negative duration would result in a Redis error. There is currently no Init() method to validate this configuration.
Consider adding:
func (r *RedisTimeSeries) Init() error {
if r.Expire != nil && time.Duration(*r.Expire) <= 0 {
return errors.New("expire must be a positive duration")
}
return nil
}Low severity since Redis would error on negative values and zero does not crash, but it can lead to a hard-to-debug configuration issue.
- Include Init() function to validate that the expire configuration option is > 0 if set - Include more descriptive explanation on the exact behavior on the expire option
|
@skartikey All suggestions directly applied, thanks! |
…sion - Use "expire the key" instead of "drop the metric" in sample.conf and README since Redis operates on keys, not metrics - Use TTL > 0 instead of != -1 for more precise TTL detection in tests - Distinguish pipeline error message from direct write error message
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
4 participants
Summary
Sending metrics to redis in addition to a database seems like a great way to boost performance, since one can offload querying to redis. In some cases it's important to be able to detect on the output side whether or not data collection / transferral is "down", so we might want to set expiry to a relatively short value, such that the value is gone if there were no metrics reported for a while. This can enable observers of the data to easily notice that something went wrong without having to check service statuses or logs or similar.
Checklist
Related issues
resolves #18336