Add new Lucene directory to track Lucene files read access usages #50283

tlrx · 2019-12-17T16:30:21Z

Note: this pull request targets the feature/searchable-snapshotsbranch

This pull request adds a new StatsDirectoryWrapper which is a Lucene FilterRepository that provides various statistics about the files it opens. It works by maintaining "live" stats for every file it opens and by wrapping all IndexInput it creates so that any operation on them (or on their slices or clones) is susceptible to contribute to the file statistics.

The directory provides two methods (getStats() and getStatsOrNull(fileName)) to capture the statistics for all files or a specific file at a given time, returning an IndexInputStats object.

This object contains information about the number of times the file has been opened, closed, sliced and cloned. It also contains information about read operations and makes the distinction between contiguous (sequential) bytes reads and non-contiguous bytes reads. For each (non-)contiguous type of reads it maintains the number of times the operation has been made, the total number of bytes read and the minimum and maximum length of bytes read at a time. Contiguous and non-contiguous stats are also aggregated under total reads stats.

Similarly to reads, IndexInputStats provides information on seek operations and makes the distinction between forward and backward seeks. For each type of seeking it maintains the number of times the operation has been made, the total number of "skipped" bytes, the min and max and average seeking distance. Both forward and backward seeks stats are also aggregated under total seeks statistics.

Finally, StatsDirectoryWrapper maintains "live" stats so that it can report (on a best effort basis) statistics on currently opened IndexInputs. A follow-up PR will be to expose those stats using a dedicated REST API.

elasticmachine · 2019-12-17T16:30:23Z

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

…wrapper

tlrx · 2019-12-18T10:06:34Z

...hable-snapshots/src/main/java/org/elasticsearch/index/store/stats/StatsDirectoryWrapper.java

+ */
+public class StatsDirectoryWrapper extends FilterDirectory {
+
+    private final Map<String, LiveStats> records = ConcurrentCollections.newConcurrentMap();


I should have added a precision here. The current implementation tracks "live" stats for IndexInputs, meaning that stats can be reported while they are actively updated. It allows to track progress while index inputs are used which I find interesting with IndexInput that can be slow to read. But it comes with additional burden since LiveStats must be used concurrently and captured on demand.

Another option would be to allow each IndexInput to update its own stats and only merge them when the IndexInput is closed.

👍 I think this is a good choice, because I think the updating of the stats should be a good deal cheaper than the operations for which we're collecting stats.

DaveCTurner

Looks good; I left two minor comments.

DaveCTurner · 2019-12-18T10:30:32Z

...hable-snapshots/src/main/java/org/elasticsearch/index/store/stats/StatsDirectoryWrapper.java

+        try {
+            super.close();
+        } finally {
+            records.clear();


If we discard the Directory immediately then it makes no difference whether we clear this or not, but if we keep hold of the closed Directory (e.g. for further analysis) then I think it would be useful to keep its stats in place.

Right, let's not remove stats on directory closing.

DaveCTurner · 2019-12-18T10:41:05Z

...hable-snapshots/src/main/java/org/elasticsearch/index/store/stats/StatsDirectoryWrapper.java

+            if (n >= 0) {
+                forwardSeeks.add(n);
+            } else {
+                backwardSeeks.add(n != Long.MIN_VALUE ? -n : 0L);


I wonder if we should just backwardSeeks.add(n); for simplicity, and deal with the negative total elsewhere.

I also think it might be useful to distinguish forward seeks by size. One possibility would be to set a threshold to distinguish large and small seeks.

I wonder if we should just backwardSeeks.add(n); for simplicity, and deal with the negative total elsewhere.

Agreed.

I also think it might be useful to distinguish forward seeks by size. One possibility would be to set a threshold to distinguish large and small seeks.

Yes. I added the length of the index input to the stats (it is useful to interpret the stats) and I made the distinction between small and large seeks (I picked < 25% of length as a threshold)

jimczi

I left some comments but the idea makes sense to me. If the purpose is to expose these stats for searchable snapshots only I wonder if this would fit more naturally in the SearchableSnapshotDirectory and SearchableSnapshotIndexInput directly since they use a buffered access pattern ?

jimczi · 2019-12-18T11:21:33Z

...hable-snapshots/src/main/java/org/elasticsearch/index/store/stats/StatsDirectoryWrapper.java

+        /**
+         * The last read position is kept around in order to detect (non)contiguous reads
+         **/
+        private final AtomicLong lastReadPosition;


Why do you need an AtomicLong ?

It is not needed, I thought I removed it already but 🤦‍♂️

jimczi · 2019-12-18T11:29:00Z

...hable-snapshots/src/main/java/org/elasticsearch/index/store/stats/StatsDirectoryWrapper.java

+        public void seek(final long pos) throws IOException {
+            final long filePointer = getFilePointer();
+            input.seek(pos);
+            stats.incrementSeekCount(pos - filePointer);


I wonder if this works with buffered index input that use seekInternal and readInternal to access the underlying input ? The count could be misleading since it doesn't take into account the internal seek that the buffered input imposes ?

I agree the count could be misleading.

My initial wish was to have a filter directory that tracks stats and that could wrap any other directory, so that we could capture stats at different layers. For example, capturing stats at the top level searchable snapshot directory or capturing stats at another filter directory (that could take care of caching index input parts etc). I'm still struggling to follow this design though.

Knowing how Lucene access the top level directory is very interesting and correlate the numbers with the internal buffered reads executed by SearchableSnapshotIndexInput is also nice to have so I followed your suggestion and moved stats into the searchable snapshot dir and input. It allows to push the LiveStats object further down in case we need to easily track other things.

tlrx · 2019-12-19T14:14:49Z

@DaveCTurner @jimczi Thanks for your comments and sorry for the time it took me to address them. I went back and forth with changes and finally fold the StatsDirectoryWrapper into the existings SearchableSnapshotDirectory and SearchableSnapshotIndexInput. This is something we could revert later but for now I think it allows to easily track what we want in the current implementation.

tlrx · 2020-02-07T08:49:16Z

Instrumentation has been added in #51637 and #51815, so I'm closing this one.

Thanks @jimczi and @DaveCTurner for your feedback here which has been incorporated to the two PRs I mentioned above.

Add StatsDirectoryWrapper

f31d3a3

tlrx added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Dec 17, 2019

tlrx requested review from DaveCTurner, ywelsch and jimczi December 17, 2019 16:30

Merge branch 'feature/searchable-snapshots' into add-stats-directory-…

18635aa

…wrapper

tlrx commented Dec 18, 2019

View reviewed changes

DaveCTurner reviewed Dec 18, 2019

View reviewed changes

jimczi reviewed Dec 18, 2019

View reviewed changes

tlrx added 6 commits December 18, 2019 14:48

Add index input length to stats

d0deaa0

Remove clear stats on close

75d9c00

allow negatives in bw seeks

a1d1340

Remove AtomicLong

96bb89a

Move stats into SearchableSnapshotDirectory

d603095

Track buffered reads

cd06905

DaveCTurner mentioned this pull request Jan 14, 2020

Lazy snapshot restores #50999

Closed

19 tasks

tlrx closed this Feb 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add new Lucene directory to track Lucene files read access usages #50283

Add new Lucene directory to track Lucene files read access usages #50283

Uh oh!

tlrx commented Dec 17, 2019

Uh oh!

elasticmachine commented Dec 17, 2019

Uh oh!

tlrx Dec 18, 2019

Uh oh!

DaveCTurner Dec 18, 2019

Uh oh!

DaveCTurner left a comment

Uh oh!

DaveCTurner Dec 18, 2019

Uh oh!

tlrx Dec 19, 2019

Uh oh!

DaveCTurner Dec 18, 2019

Uh oh!

tlrx Dec 19, 2019

Uh oh!

jimczi left a comment

Uh oh!

jimczi Dec 18, 2019

Uh oh!

tlrx Dec 19, 2019

Uh oh!

jimczi Dec 18, 2019

Uh oh!

tlrx Dec 19, 2019

Uh oh!

tlrx commented Dec 19, 2019

Uh oh!

tlrx commented Feb 7, 2020

Uh oh!

Uh oh!

Add new Lucene directory to track Lucene files read access usages #50283

Add new Lucene directory to track Lucene files read access usages #50283

Uh oh!

Conversation

tlrx commented Dec 17, 2019

Uh oh!

elasticmachine commented Dec 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Dec 19, 2019

Uh oh!

tlrx commented Feb 7, 2020

Uh oh!

Uh oh!