Add item_count metric to field data cache stats API

[peteralfonsi]

Description

Adds an item_count metric to the existing _nodes/stats/indices/fielddata API. Like memory size, it can be aggregated by field by passing a fielddata_fields param listing the desired fields.

Example API response for curl -XGET "localhost:9200/_nodes/stats/indices/fielddata?pretty&fielddata_fields=text0,text2":

"indices" : {
        "fielddata" : {
          "memory_size_in_bytes" : 31184,
          "evictions" : 0,
          "item_count" : 10,
          "fields" : {
            "text0" : {
              "memory_size_in_bytes" : 15580,
              "item_count" : 5
            },
            "text2" : {
              "memory_size_in_bytes" : 15604,
              "item_count" : 5
            }
          }
        }
      }

Related Issues

Resolves #19173

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

❌ Gradle check result for b56cbf2: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for dce88a7: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for a297d35: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[peteralfonsi]

Hmm, I had mistakenly renamed a PublicApi method so detect-breaking-change was failing. But now that I've fixed it the job seems to be having some issue with Failed to load eclipse jdt formatter: run 1, run 2

[github-actions]

❌ Gradle check result for 91cbbc9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for f1d7105: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 1dda2ac: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[sgup432]

@peteralfonsi
The current approach looks very hacky with lots of custom handling of cases(like memory etc), will be harder to maintain in the future which defeats the initial purpose of writing in a generic way.

I tried writing FieldStats logic in a generic way(have removed some unimportant methods). The idea was to maintain a FieldNames enum to store desired information. You don't necessarily need to do this way but gives an idea. Looks more clean.

public class FieldStats  implements Writeable, Iterable<Map.Entry<String, Long>> {
    private final Map<String, Long> stats;

    /**
     * Creates a new FieldMemoryStats instance
     */
    public FieldStats(Map<String, Long> stats) {
        this.stats = Objects.requireNonNull(stats, "status must be non-null");
        assert stats.containsKey(null) == false;
    }

    /**
     * Creates a new FieldMemoryStats instance from a stream
     */
    public FieldStats(StreamInput input) throws IOException {
        Map<String, Long> tempStats = new HashMap<>();
        for (FieldNames fieldNames: FieldNames.values()) {
            if (fieldNames.getVersion().onOrAfter(input.getVersion())) {
                tempStats.put(fieldNames.key, input.readVLong());
            }
        }
        this.stats = tempStats;
        //stats = input.readMap(StreamInput::re, StreamInput::readVLong);
    }


    @Override
    public void writeTo(StreamOutput out) throws IOException {
        for (FieldNames fieldNames: FieldNames.values()) {
            if (fieldNames.getVersion().onOrAfter(out.getVersion())) {
                out.writeVLong(stats.get(fieldNames.key));
            }
        }
    }

    /**
     * Generates x-content into the given builder for each of the fields in this stats instance
     * @param builder the builder to generated on
     * @param key the top level key for this stats object
     */
    public void toXContent(XContentBuilder builder, String key) throws IOException {
        builder.startObject(key);
        for (final var entry : stats.entrySet()) {
            String key1 = entry.getKey();
            FieldNames fieldNames = FieldNames.valueOf(key1);
            builder.startObject(key1);
            if (fieldNames.getReadableKey() != null) {
                builder.humanReadableField(fieldNames.key, fieldNames.readableKey, new ByteSizeValue(entry.getValue()));
            } else {
                builder.field(fieldNames.key, entry.getValue());
            }
            builder.endObject();
        }
        builder.endObject();
    }


    public enum FieldNames {
        MEMORY("memory_size", "memory_size_in_bytes", Version.V_EMPTY),
        ITEM_COUNT("item_count", null, Version.V_3_3_0);

        String key;
        String readableKey;
        Version version;

        FieldNames(String key, String readableKey, Version version) {
            this.key = key;
            this.readableKey = readableKey;
            this.version = version;
        }

        public Version getVersion() {
            return this.version;
        }

        public String getReadableKey() {
            return readableKey;
        }

        public String getKey() {
            return key;
        }
    }
}

FieldDataStats also looks more clean now

public class FieldDataStats implements Writeable, ToXContentFragment {

    private static final String FIELDDATA = "fielddata";
    private static final String MEMORY_SIZE = "memory_size";
    private static final String MEMORY_SIZE_IN_BYTES = "memory_size_in_bytes";
    private static final String EVICTIONS = "evictions";
    private static final String FIELDS = "fields";
    private long memorySize;
    private long itemCount;
    private long evictions;
    @Nullable
    private FieldStats fields;

    public FieldDataStats() {

    }

    public FieldDataStats(StreamInput in) throws IOException {
        memorySize = in.readVLong();
        evictions = in.readVLong();
        if (in.getVersion().onOrAfter(FieldStats.FieldNames.ITEM_COUNT.getVersion())) {
            itemCount = in.readVLong();
        }
        fields = in.readOptionalWriteable(FieldStats::new);
    }

    public FieldDataStats(long memorySize, long evictions, @Nullable FieldStats fields) {
        this(memorySize, evictions, fields, 0);

    }

    public FieldDataStats(long memorySize, long evictions, @Nullable FieldStats fields, long itemCount) {
        this.memorySize = memorySize;
        this.evictions = evictions;
        this.fields = fields;
        this.itemCount = itemCount;

    }

    public void add(FieldDataStats stats) {
        if (stats == null) {
            return;
        }

        this.memorySize += stats.memorySize;
        this.evictions += stats.evictions;
        this.itemCount += stats.itemCount;
        if (stats.fields != null) {
            if (fields == null) {
                fields = stats.fields.copy();
            } else {
                fields.add(stats.fields);
            }
        }
    }


    @Nullable
    public FieldStats getFields() {
        return fields;
    }

    @Override
    public void writeTo(StreamOutput out) throws IOException {
        out.writeVLong(memorySize);
        out.writeVLong(evictions);
        out.writeOptionalWriteable(fields);
        if (out.getVersion().onOrAfter(FieldStats.FieldNames.ITEM_COUNT.getVersion())) {
            out.writeVLong(itemCount);
        }
    }

    @Override
    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
        builder.startObject(FIELDDATA);
        builder.humanReadableField(MEMORY_SIZE_IN_BYTES, MEMORY_SIZE, getMemorySize());
        builder.field(EVICTIONS, getEvictions());
        if (fields != null) {
            fields.toXContent(builder, FIELDS);
        }
        builder.endObject();
        return builder;
    }

See if you reuse above and write it in a much better way!

[github-actions]

❌ Gradle check result for c9109fc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 6dd05d0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[peteralfonsi]

Fixed a horrifying git rebase disaster...should now contain only the necessary commits for the simpler implementation.

[github-actions]

❌ Gradle check result for e6cc019: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[sgup432]

We shouldn't be doing this. Using FieldMemoryStats logic for both itemCount and memory is what we don't want. As FieldMemoryStats is specific to memory related stats and shouldn't be used for other stuff.

Better to do it the original way?

 if (fieldMemorySizes != null) {
            fieldMemorySizes.toXContent(builder, FIELDS, MEMORY_SIZE_IN_BYTES, MEMORY_SIZE);
}
if (fieldItemCounts != null) {
  fieldItemCounts.toXContent()
}

[peteralfonsi]

No, like we talked about we can't use each individual object's XContent method separately, since the objects don't know about each other and then the resulting xcontent would be grouped like:

which is incorrect. So we need a static helper method which will group the output by field as the top level.

The logic in the helper method is not specific to FieldMemoryStats at all, it's just in that file since it seemed like a logical place to put it. I guess if we have them both extend from some base class it could go there instead.

[github-actions]

❌ Gradle check result for 7ac9b10: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.