API for listing index file sizes

[camilojd]

I'd like to propose this API to query index file sizes, as part of SegmentsStats.

Copypasting a comment previously posted on #16131:

Some comments/questions follow.

I'm consolidating file sizes using the same criteria of SegmentsStats to expose disk usage of Lucene index files: Terms, TermVectors, StoredFields, Norms, DocValues (class IndexResources in SegmentsStats).

SegmentsStats currently exposes memory consumption as bytes of each of the aforementioned so it's possible to add these as part of a class (IndexResources) and expose it as two fields of SegmentsStats, one for memory resources (existing stats) and other for disk resources. This is what I implemented in my branch, but I'm not sure if it would be better to expose this as another "sister" class of SegmentsStats, member of CommonStats, and leave SegmentsStats as it is. I'm however aware that my toXContent serialization could break clients expecting certain fields that now are placed elsewhere.

Some other couple questions I've got:

1. Is it necessary to consolidate sizes by the same criteria of SegmentsStats or may be better doing it using the file extensions? This last option has the advantage that would include additional information about postings, i.e., separate size info for positions, payloads, etc.
2. The meat of the matter: Is the way I open the Directory from the SegmentInfo and the compound reader a sensible approach? I'm not sure if this could introduce issues with the Store.

Closes #16131

[clintongormley]

@mikemccand would you mind having a look at this PR please?

[mikemccand]

Hmm should we have an "other" category in a final else clause here?

[mikemccand]

Is the way I open the Directory from the SegmentInfo and the compound reader a sensible approach?

Yes, that's correct!

Is it necessary to consolidate sizes by the same criteria of SegmentsStats or may be better doing it using the file extensions? This last option has the advantage that would include additional information about postings, i.e., separate size info for positions, payloads, etc.

I think I'd rather see by file extension? E.g. knowing whether your terms dict is huge, or your docs or positions or payloads are huge, is important information.

We could do extensions plus an "interpretation" e.g. ".tim (terms)", ".tip (terms index)", etc.? Doing it this way also has the advantage that an unknown extension (which can easily happen when lucene changes its file format, e.g. the upcoming dimensional points is a change in 6.0) can just be added into the stats.

[camilojd]

We could do extensions plus an "interpretation" e.g. ".tim (terms)", ".tip (terms index)", etc.? Doing it this way also has the advantage that an unknown extension (which can easily happen when lucene changes its file format, e.g. the upcoming dimensional points is a change in 6.0) can just be added into the stats.

I like this compromise. I'll simplify some things and proceed to fix the mentioned issues. 👍

[camilojd]

@mikemccand Hi Mike,

I fixed all of the above, and some more. Details:

• Simplified field: just a ImmutableOpenMap<String, Long> to store per extension sizes.
• Changed exception handling to limit try/catch blocks to the minimum scope, logging with warn when necessary
• Transport serialization doesn't serialize the map field until ES Version 3.0
• XContent serialization takes care of handling known/unknown extension descriptions by querying a map.
• Querying segment disk usage is optional: I added a boolean flag to CommonStatsFlags, triggered by setting a detailed_segments parameter in the REST endpoint to query segments disk stats. I don't know if there is a nicer way to do this.

So far it works and passes all the tests.

It seems to me that a better place for this would be under StoreStats, because Store already queries the effective size of all physical files and caches the result... but had no idea/luck on how to grab a SegmentInfo for uncommitted segments (only does show some disk usage after forcing a _flush). See camilojd@df0370f

[mikemccand]

Maybe add an else clause here to set detailedSegmentsStats to false so it's explicit?

[mikemccand]

Thanks @camilojd, this is looking very nice. I just left some minor comments.

Querying segment disk usage is optional: I added a boolean flag to CommonStatsFlags, triggered by setting a detailed_segments parameter in the REST endpoint to query segments disk stats. I don't know if there is a nicer way to do this.

I think CommonStatsFlags is fine for now: I don't know of another place to put it (maybe someone else has a suggestion?).

Do we also need to punch the new boolean option through to e.g. the Java client API (and eventually the other language clients)?

It seems to me that a better place for this would be under StoreStats, because Store already queries the effective size of all physical files and caches the result... but had no idea/luck on how to grab a SegmentInfo for uncommitted segments (only does show some disk usage after forcing a _flush).

I do agree it would be more natural to have the stats there, e.g. we could just fix store stats to break out the file sizes by extension, and not cause any additional IO load to the filesystem.

Maybe it would be an OK limitation that it'd only list flushed segments? We could explain that in the docs?

[camilojd]

@mikemccand just pushed changes that address all your comments. I really like how the code became more self-contained. :-)

Do we also need to punch the new boolean option through to e.g. the Java client API (and eventually the other language clients)?

The option is in the Client API, accessible through IndicesStatsRequest.detailedSegmentsStats(boolean), so also through NodesStatsRequest.indices.

Currently it can be requested through HTTP in RestIndicesStatsAction and RestNodesStatsAction, but it's kinda ugly, just checks if the key detailed_segments is in the query string. Maybe leave the option only in the Client API by now?

Maybe it would be an OK limitation that it'd only list flushed segments? We could explain that in the docs?

It's possible to do that, but it's kind of weird. Currently StoreStats already informs the real disk usage of the directory, but numbers in my implementation will almost always be very far from the total, because of unflushed segments. SegmentsStats is more accurate (not exactly 100% accurate because of cfs, etc).

[mikemccand]

Thanks @camilojd, I'll review your latest changes.

The option is in the Client API, accessible through IndicesStatsRequest.detailedSegmentsStats(boolean), so also through NodesStatsRequest.indices.

Oh good, sorry I missed this.

Currently it can be requested through HTTP in RestIndicesStatsAction and RestNodesStatsAction, but it's kinda ugly, just checks if the key detailed_segments is in the query string. Maybe leave the option only in the Client API by now?

I think it's good to expose the option in the REST API too.

SegmentsStats is more accurate (not exactly 100% accurate because of cfs, etc).

OK let's leave it where it is now, but maybe add a TODO about whether this could/should move to StoreStats instead, explaining the tradeoffs we discussed here?

[mikemccand]

Hmm can we rename detailedSegmentStats? Maybe includeSegmentFileSizes?

[mikemccand]

This change looks great!

I just left a minor (naming, the hardest part!) comment, and let's add a TODO about maybe moving this to StoreStats in the future?

Can you add some tests here to confirm the feature is working and catch us if anyone breaks it in the future, and also update the docs explaining this new cool parameter? Thanks @camilojd!

[camilojd]

@mikemccand all comments were addressed.

Do you think there is anything left to improve?
Thanks for the amazing help!

[mikemccand]

Can you update docs/reference/indices/stats.asciidoc with this new parameter?

[mikemccand]

I think we just need to document the new parameter and then we are done! Thanks @camilojd.

[camilojd]

@mikemccand now it's done! 👍 Thank you sir!

[mikemccand]

Hmm I just realized we are doing too much work here? We should only be visiting the files belonging to this segment, but we are instead visiting all files in the directory (at least, in the non-compound-file case)?

I think instead of directory.listAll(), you could use segmentReader.getSegmentInfo().files()? This should return all file names that this segment uses ...

[camilojd]

I think instead of directory.listAll(), you could use segmentReader.getSegmentInfo().files()? This should return all file names that this segment uses ...

Hi @mikemccand,
Makes sense. Perhaps we can do:

        if (useCompoundFile) {
            files = directory.listAll();
        } else {
            files = segmentReader.getSegmentInfo().files().toArray(new String[]{});
        }

Otherwise getSegmentInfo.files() returns .cfs, .cfe files and the Compound Directory can't find them when querying fileLength

[mikemccand]

Ahh yes @camilojd I agree that makes sense!

[mikemccand]

Thanks @camilojd I left one more comment!

[camilojd]

@mikemccand I updated the code according to the comments :-) thanks!

Edit: fixed the exception handling to log the correct messages and limit the try {} scope to one line at most.

[mikemccand]

Thanks @camilojd, this looks great, I'll push to 5.0 now!

[camilojd]

Awesome! thank you very much @mikemccand!!