Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2024-05-02 Thread Lucas Brutschy
Hi Nick! I agree, the age variant is a bit nicer since the semantics are very clear from the name. If you'd rather go for the simple implementation, how about calling it `oldest-iterator-open-since-ms`? I believe this could be understood without docs. Either way, I think we should be able to open

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2024-03-31 Thread Nick Telford
Hi Matthias, > For the oldest iterator metric, I would propose something simple like > `iterator-opened-ms` and it would just be the actual timestamp when the > iterator was opened. I don't think we need to compute the actual age, > but user can to this computation themselves? That works for me;

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2024-03-31 Thread Matthias J. Sax
The time window thing was just an idea. Happy to drop it. For the oldest iterator metric, I would propose something simple like `iterator-opened-ms` and it would just be the actual timestamp when the iterator was opened. I don't think we need to compute the actual age, but user can to this

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2024-03-28 Thread Nick Telford
Quick addendum: My suggested metric "oldest-open-iterator-age-seconds" should be "oldest-open-iterator-age-ms". Milliseconds is obviously a better granularity for such a metric. Still accepting suggestions for a better name. On Thu, 28 Mar 2024 at 13:41, Nick Telford wrote: > Hi everyone, > >

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2024-03-28 Thread Nick Telford
Hi everyone, Sorry for leaving this for so long. So much for "3 weeks until KIP freeze"! On Sophie's comments: 1. Would Matthias's suggestion of a separate metric tracking the age of the oldest open iterator (within the tag set) satisfy this? That way we can keep iterator-duration-(avg|max) for

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2024-03-13 Thread Sophie Blee-Goldman
About your last two points: I completely agree that we should try to make this independent of RocksDB, and should probably adopt a general philosophy of being store-implementation agnostic unless there is good reason to focus on a particular store type: eg if it was only possible to implement for

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2024-03-07 Thread Matthias J. Sax
Seems I am late to this party. Can we pick this up again aiming for 3.8 release? I think it would be a great addition. Few comments: - I think it does make sense to report `iterator-duration-avg` and `iterator-duration-max` for all *closed* iterators -- it just seems to be a useful metric

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-25 Thread Sophie Blee-Goldman
> > If we used "iterator-duration-max", for > example, would it not be confusing that it includes Iterators that are > still open, and therefore the duration is not yet known? 1. Ah, I think I understand your concern better now -- I totally agree that a "iterator-duration-max" metric would be

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-24 Thread Nick Telford
I don't really have a problem with adding such a metric, I'm just not entirely sure how it would work. If we used "iterator-duration-max", for example, would it not be confusing that it includes Iterators that are still open, and therefore the duration is not yet known? When graphing that over

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-24 Thread Sophie Blee-Goldman
I actually think we could implement Lucas' suggestion pretty easily and without too much additional effort. We have full control over the iterator that is returned by the various range queries, so it would be easy to register a gauge metric for how long it has been since the iterator was created.

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-18 Thread Lucas Brutschy
Hi Nick, I did not think in detail about how to implement it, just about what metrics would be nice to have. You are right, we'd have to register/deregister the iterators during open/close. This would be more complicated to implement than the other metrics, but I do not see a fundamental problem

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-17 Thread Nick Telford
Hi Lucas, Hmm, I'm not sure how we could reliably identify such leaked Iterators. If we tried to include open iterators when calculating iterator-duration, we'd need some kind of registry of all the open iterator creation timestamps, wouldn't we? In general, if you have a leaky Iterator, it

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-16 Thread Lucas Brutschy
Hi Nick! thanks for the KIP! I think this could be quite useful, given the problems that we had with leaks due to RocksDB resources not being closed. I don't have any pressing issues why we can't accept it like it is, just one minor point for discussion: would it also make sense to make it

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-05 Thread Nick Telford
Hi Colt, I kept the details out of the KIP, because KIPs generally document high-level design, but the way I'm doing it is like this: final ManagedKeyValueIterator rocksDbPrefixSeekIterator = cf.prefixScan(accessor, prefixBytes); --> final long startedAt = System.nanoTime();

Re: [DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-05 Thread Colt McNealy
Thank you for the KIP, Nick! This would be highly useful for many reasons. Much more sane than checking for leaked iterators by profiling memory usage while running 100's of thousands of range scans via interactive queries (: One small question: >The iterator-duration metrics will be updated

[DISCUSS] KIP-989: RocksDB Iterator Metrics

2023-10-04 Thread Nick Telford
Hi everyone, KIP-989 is a small Kafka Streams KIP to add a few new metrics around the creation and use of RocksDB Iterators, to aid users in identifying "Iterator leaks" that could cause applications to leak native memory. Let me know what you think!