fix: fix group by terms

[rtpg]

The plugin would allow for p95 metrics, but then, when referencing them, would look up by "p95.0"s.

Renaming p95s to p95.0s seems to be an elastic search oddity that Quickwit doesn't do

[uptickmetachu]

Tested this end to end against a local stack (Quickwit 0.8.0-nightly + Grafana 13.0.2, plugin built from this branch, 600 spans ingested into otel-traces-v0_9). The change doesn't fix the query — it swaps one error for the same error:

OLD  orderBy=1[95.0]  ->  could not find aggregation with name 1[95  in metric sub_aggregations
NEW  orderBy=1[95]    ->  could not find aggregation with name 1[95] in metric sub_aggregations

Explore still shows the error and returns no data.

Why

The .0 suffix isn't the problem — the [...] bucket-path syntax is. Quickwit hands the aggregation JSON straight to tantivy, and tantivy parses a terms order key as <aggName>[.<property>], splitting on the first dot only:

// tantivy src/aggregation/bucket/term_agg/mod.rs
pub(crate) fn get_agg_name_and_property(name: &str) -> (&str, &str) {
    let (agg_name, agg_property) = name.split_once('.').unwrap_or((name, ""));
    (agg_name, agg_property)
}

1[95] never splits, so the whole string becomes the aggregation name and the lookup fails. Elasticsearch's metric[path] form does not exist here.

And even with a path that does resolve, percentiles are refused outright:

// tantivy src/aggregation/agg_result.rs
MetricResult::Percentiles(_) => Err(TantivyError::AggregationError(
    AggregationError::InvalidRequest("percentiles can't be used to order".to_string()),
)),

Same treatment as top_hits. Probing every spelling against a live Quickwit confirms there is no way past both gates:

1[95]        FAIL  could not find aggregation with name 1[95]
1[95.0]      FAIL  could not find aggregation with name 1[95
1>95         FAIL  could not find aggregation with name 1>95
1.95         FAIL  percentiles can't be used to order
1.p95        FAIL  percentiles can't be used to order
1.values.95  FAIL  percentiles can't be used to order
1            FAIL  percentiles can't be used to order

So ordering terms by a percentile is not achievable in Quickwit at all, at any spelling. The option has to go, not be reformatted. I've left suggestions doing that.

Second bug this uncovered

createOrderByOptionsForExtendedStats emits the same broken bracket form, so extended stats ordering is equally dead on main today. That one is fixable — tantivy's ExtendedStats::get_value accepts real properties, they just need the dot form:

OK    1.avg
OK    1.max
OK    1.std_deviation
OK    1.std_deviation_bounds.lower
OK    1.std_deviation_bounds.upper
FAIL  1[avg]         could not find aggregation with name 1[avg]
FAIL  1[std_lower]   could not find aggregation with name 1[std_lower]

That function is outside this diff so I can't attach a suggestion to it, but the change is:

+const extendedStatOrderProperty: Record<ExtendedStatMetaType, string> = {
+  avg: 'avg',
+  min: 'min',
+  max: 'max',
+  sum: 'sum',
+  count: 'count',
+  std_deviation: 'std_deviation',
+  std_deviation_bounds_lower: 'std_deviation_bounds.lower',
+  std_deviation_bounds_upper: 'std_deviation_bounds.upper',
+};
+
 function createOrderByOptionsForExtendedStats(metric: ExtendedStats): SelectableValue<string> {
   ...
     .map((key) => {
-      let method = key as string;
-      if (key === 'std_deviation_bounds_lower') {
-        method = 'std_lower';
-      }
-      if (key === 'std_deviation_bounds_upper') {
-        method = 'std_upper';
-      }
-      return { label: `${describeMetric(metric)} (${method})`, value: `${metric.id}[${method}]` };
+      const property = extendedStatOrderProperty[key];
+      return { label: `${describeMetric(metric)} (${property})`, value: `${metric.id}.${property}` };
     });

Summary

metric	orderable?	correct key
avg / sum / min / max	yes	1
cardinality	yes	1
extended_stats	yes	1.avg, 1.std_deviation_bounds.lower
percentiles	no	impossible — remove from options
top_metrics	no	already excluded
_count / _key	yes	unchanged

Tidier form

The suggestion below leaves createOrderByOptionsForPercentiles as a stub because suggestions can only touch lines in this diff. Cleaner is to delete the function and its branch in createOrderByOptions, and gate it where top_metrics is already gated:

 function isValidOrderTarget(metric: MetricAggregation) {
   return (
     // top metrics can't be used for ordering
     metric.type !== 'top_metrics' &&
+    // Quickwit rejects percentiles as an order target outright ("percentiles can't be used to
+    // order"), whatever bucket path is used, so don't offer them.
+    metric.type !== 'percentiles' &&
     // pipeline aggregations can't be used for ordering: ...
     !isPipelineAggregation(metric)
   );
 }

Verified on the same local stack: the Order By dropdown for a percentiles metric then offers only Term value and Doc Count, extended stats offers 1.avg / 1.std_deviation_bounds.lower / 1.std_deviation_bounds.upper, and selecting one returns a populated table with no error.

[uptickmetachu]

Percentiles can't be an order target in Quickwit at all, so this should return nothing rather than a differently-spelled path.

Suggested change

	return metric.settings.percents.map((percent) => {
	// The bucket path for percentile numbers is appended with a `.0` if the number is whole
	// otherwise you have to use the actual value.
	const percentString = /^\d+\.\d+/.test(`${percent}`) ? percent : `${percent}.0`;
	return { label: `${describeMetric(metric)} (${percent})`, value: `${metric.id}[${percentString}]` };
	// Unlike Elasticsearch, Quickwit's aggregation engine names the percentile
	// sub-aggregation after the percent value exactly as sent in the request
	// (e.g. "95"), without appending `.0` for whole numbers.
	return { label: `${describeMetric(metric)} (${percent})`, value: `${metric.id}[${percent}]` };
	});
	}
	// Quickwit refuses percentiles as an order target whatever bucket path is used, failing with
	// `InvalidRequest: "percentiles can't be used to order"`, so don't offer them at all.
	return [];
	}

See the review body for the tidier version, which drops this function entirely and gates percentiles in isValidOrderTarget.

[uptickmetachu]

This asserts the broken behaviour: 1[95] is exactly the value Quickwit rejects. Worth asserting no percentile option is produced instead.

Suggested change

	it('Order by option value for a whole-number percentile should not append ".0"', () => {
	// Quickwit's aggregation engine names the percentile sub-aggregation after the
	// percent value exactly as sent in the request (e.g. "95"), unlike Elasticsearch
	// which requires a ".0" suffix for whole-number bucket paths (e.g. "95.0").
	// Sending an orderBy of "1[95.0]" makes Quickwit fail with
	// "could not find aggregation with name 1[95] in metric sub_aggregations".
	const percentiles: Percentiles = { id: '1', type: 'percentiles', field: '@value', settings: { percents: ['95'] } };
	const options = createOrderByOptions([percentiles]);
	expect(options.some((option) => option.value === '1[95]')).toBe(true);
	expect(options.some((option) => option.value === '1[95.0]')).toBe(false);
	});
	it('Percentiles should not be in "order by" options', () => {
	// Quickwit refuses percentiles as an order target whatever bucket path is used, failing with
	// `InvalidRequest: "percentiles can't be used to order"`, so they must not be offered at all.
	const percentiles: Percentiles = { id: '1', type: 'percentiles', field: '@value', settings: { percents: ['95'] } };
	const options = createOrderByOptions([percentiles]);
	expect(options.every((option) => !option.value?.startsWith('1'))).toBe(true);
	});