timeslist does not expire about sidekiq-statistic HOT 16 CLOSED

@davydovanton I've put this gem on the "troublesome" list and removed it from the Related Projects wiki page since this bug hasn't been fixed.

from sidekiq-statistic.

+1, Redis killed by the Linux Out of Memory Killer.

As a workaround, I clear all the timeslist on a regular basis like this:

Sidekiq.redis do |conn|
  conn.keys('*:timeslist').each { |key| conn.del(key) }
end

from sidekiq-statistic.

Sorry for the delay @drn.

I was actually referring to this piece of code: https://github.com/davydovanton/sidekiq-statistic/blob/master/lib/sidekiq/statistic/statistic/metrics/store.rb#L52

It was added in the past and since then we haven't seen activities here, that's the reason I closed the issue.

We should probably have other approaches to handle it and I really think we can discuss further, if that's the case we may want to create a new issue to change what we actually have.

from sidekiq-statistic.

@MrJoy This did seem to address the issue we were having, yes. It safely drops (very) old history once an upper bound is reached, so perhaps not the most elegant solution, but works for us while still rendering the tool very useful.

from sidekiq-statistic.

+1 constantly growing redis memory usage became an issue for us due to sidekiq-statistic

from sidekiq-statistic.

Maybe hyperloglog can be used to aggregate execution counts over longer periods. Redis' hyperloglog is exact for smaller counts and can be merged to aggregate data. So it would be possible to store counts per day/hour/whatever and then aggregate to bigger intervals. Eg. daily merge per hour to per day, per day to week and merge to total.

This only works for counting things and not for storing durations and all the possible keys have to be stores separately. But it's probably OK to have average execution time over a bounded period (eg. last month) and execution count per status unbounded using hll.

from sidekiq-statistic.

+1 I just tracked down our out of control memory growth to this!

from sidekiq-statistic.

@mperham yeah I think that would save everybody a lot of trouble.

from sidekiq-statistic.

That's a major issue, anyone tried to fixed that? I have to stop using this lib until it's fixed.

from sidekiq-statistic.

Lots more could be done to re-vamp how times are calculated and stored, but I think the solution I've got up at #107 could bring this gem back to stable and performant for those running a lot of jobs.

from sidekiq-statistic.

@garethson Does that mean this issue should be be closed, with a follow-up issue to improve storage efficiency? Or is there still a problem of unbounded memory growth after your patch?

from sidekiq-statistic.

I'm closing this issue considering we already have a feature to get rid of pretty old info on Redis.

from sidekiq-statistic.

Hi @wenderjean — can you clarify which feature you're referring to?

from sidekiq-statistic.

He's referring to expiration times. Redis can act as an LRU cache either globally, or explicitly per-value when an expiration is set.

from sidekiq-statistic.

Thanks for clarifying, @MrJoy

This may have changed recently, but my understanding is that there is no option to specify a global TTL in Redis and it must be specified per key. It looks like Redis 4.0 introduced some new functionality — https://redis.io/topics/lru-cache, but the eviction policies seem geared for using Redis as a cache rather than a queue.

Rather than having the users of this library adjust global Redis configuration or expiring keys generated by this library explicitly, it seems like it would make sense for the library to handle setting TTLs for these keys directly.

Feel free to leave this closed (my organization no longer uses the gem), but I do think that this library could benefit from handling expirations more explicitly either by default or through configuration.

Cheers 👋

from sidekiq-statistic.

Got it, thanks for clarifying! I was mainly asking out of curiosity — this is helpful. Cheers 👋

from sidekiq-statistic.