[Rollup] Remove builders from MetricConfig

[tlrx]

Same motivation as #32507 but for the MetricConfig configuration object.

Related to #29827

[elasticmachine]

Pinging @elastic/es-search-aggs

[jimczi]

The validation of the metrics is fragile IMO. It feels wrong to detect a bad metric name in the transport level. @polyfractal I think it's time to use an Enum rather than a String in the metrics config ? Something like:

public enum Metric {
        /**
         * Pick the lowest value.
         */
        MIN(MinAggregationBuilder.NAME),

        /**
         * Pick the highest value.
         */
        MAX(MaxAggregationBuilder.NAME),

        /**
         * Use the average of all values.
         */
        AVG(AvgAggregationBuilder.NAME),

        /**
         * Use the sum of all values.
         */
        SUM(SumAggregationBuilder.NAME),

        /**
         * Use the number of values.
         */
        VALUE_COUNT(ValueCountAggregationBuilder.NAME);

        final String name;

        Metric(String name) {
            this.name = name;
        }
    }

We can add more later but the choice should be constrained to the available ones ?

[tlrx]

Thanks @jimczi.

The validation of the metrics is fragile IMO. It feels wrong to detect a bad metric name in the transport level.

Indeed, I've been too quick on this. Having an enum is the right solution and I think I saw some todos about this in the rollup codebase. This can be done as a separate change, so maybe I could just reuse the previous code that hard check the metrics names so that this PR is not blocked?

[jimczi]

sure +1 to keep the previous code and we can update to an enum in a follow up if @polyfractal agrees.

[tlrx]

Thanks @jimczi. I restored the hard checks for metrics.

[polyfractal]

Yeah, an enum would be cleaner/nicer. As soon as we started hardcoding strings I put in that TODO to stop doing it :)

[polyfractal]

Mostly just curious, but why Random as an argument instead of using the test class' random methods (randomBoolean(), etc)?

[tlrx]

I wasn't sure that ConfigTestHelpers methods were always used in the context of an ESTestCase, and I expect to have to use these methods in the high level rest client too so I found it safer to pass the Random object to the methods similarly to what is done for RandomNumbers or RandomPicks.

[polyfractal]

👍 cool, thanks for the explanation :)

[polyfractal]

Is there any advantage in randomizing these? I assumed that since unit tests are fast to execute, we should go ahead and test the obvious paths all the time so that failures aren't flaky.

[tlrx]

You're right, Jim made the same remark on another pull request. I removed the randomization.

[polyfractal]

👍

[tlrx]

Thanks @jimczi and @polyfractal