Aggregations and counter field support.

[martijnvg]

No longer offer limited aggregation support counter fields.
This extends the counter field support to all aggregation.

Unfortunately I didn't see a way to automatically register aggregators for counter fields and numeric fields at the same time.
The rate aggregation does need the distinction between an aggregator for counter and numeric fields. And implicit logic in ValuesSourceRegistry.Builder#register(...) method seems trappy.

This change registers many aggregators for both numeric and counter value types. Introduced a constants to make registering a bit easier.

Closes #93539

[elasticsearchmachine]

Hi @martijnvg, I've created a changelog YAML for you.

[elasticsearchmachine]

Pinging @elastic/es-analytics-geo (Team:Analytics)

[not-napoleon]

I think this is the wrong thing to do. This is intended to work around a "bad" user experience, but it does so by creating a worse user experience. This change says that things we know yield garbage results are explicitly and intentionally supported. We've done this before to work around UX issues, and gotten pretty badly burned. I'm thinking of terms sort ascending, which we know produces unbounded error (read as "utter garbage results that shouldn't be used for any purpose"), and yet it continues to get wide use. This is inviting the same kind of mistake, and I cannot in good faith approve it.

Strong 👎🏼

[not-napoleon]

I've heard that leniency is abhorrent, and should be eschewed. I deeply do not like having these lists. I probably shouldn't have even added ALL_CORE when first doing this, and I definitely don't want to proliferate them. Yes, it's more typing to have to list them out at registration time, but it makes it much clearer exactly what we mean.

Registration is write once, read many times. I would like to optimize for not needing to click through to the list definition every time to see exactly what types are included. I would also like for people adding new types to have to explicitly think about what aggregations should be supported.

[not-napoleon]

Thinking more about this, if we absolutely must include this to make the UI work correctly, we should write new aggregators that always return a constant (e.g. -inf) for all invalid counter aggregations. That would at least be fast, as we could generate our completely wrong answers without having to actually read any documents.

[jpountz]

I agree that neither option is great. If we're lenient, we're most likely returning nonsensical results to the user. If we're strict, users could get frustrated that Elasticsearch artificially refuses to run a computation that they want to run. The following other factors come to mind:

• It's possible to work around the restriction by creating a runtime field that returns the value of the counter field and aggregating on the runtime field. I guess this could be an argument either way, e.g. it's not that harmful we're being strict since users have a workaround vs. what's the point of this check if users can bypass it easily.
• Leniency would no longer work once the backing index has been downsampled, so being lenient doesn't fix the problem, it only pospones it. We're doing users a favor by failing fast so that they have a chance to fix their queries or mappings before future downsample operations.
• It's often not the same person who controls mappings and runs queries, could it increase the frustration in case the person who controls mappings made an error that prevents queries?
• Introducing restrictions with metric types could be an incentive not to flag metric types correctly in TSDB data streams, or not to use TSDB at all.

I dislike the cons either way, and wonder if we could find something in-between. Thinking out loud, I have the following in mind at the moment:

• Issue a warning instead of refusing the run the aggregation in Elasticsearch.
• Improve Kibana to discourage running aggregations that don't make sens on counter fields, which is an information that Kibana has since metric types are returned in field caps. E.g. the ability to do an average on a counter field could still be available but greyed out to indicate that it makes little sense.

I like that it would still let users know in case they queries disagree with their mappings regarding the fact that a field is a counter, which is what I like with the strict option, while still allowing users to run arbitrary aggregations, which is what I like about the lenient option.

[timductive]

Improve Kibana to discourage running aggregations that don't make sens on counter fields, which is an information that Kibana has since metric types are returned in field caps. E.g. the ability to do an average on a counter field could still be available but greyed out to indicate that it makes little sense.

This is what we want to do, this PR is trying to work around the issue that the Kibana integration is not ready for technical preview in 8.7. @thomasneirynck keep me honest here but my opinion is, if we are uncomfortable with this stop-gap measure then we need to revert this out of 8.7 and take 8.8 to provide the right experience in Kibana. Trying to fix this in the UI post feature freeze is high risk and a poor experience.

[thomasneirynck]

agreed @timductive . I don't think there will be an appropriate stop-gap measure we can do that would be fit for a post-FF 8.7 release.

The one we had discussed (aggregatable: false) may work, but it's a little unclear what the total fallout would be across all apps. (e.g. in Analytics, we'd see the field disappear in many UX).

The other alternative, suggested by @jpountz:

wrt:

Improve Kibana to discourage running aggregations that don't make sens on counter fields, which is an information that Kibana has since metric types are returned in field caps. E.g. the ability to do an average on a counter field could still be available but greyed out to indicate that it makes little sense

To do this right is imho beyond what we can do as a bug fix for 8.7.

[thomasneirynck]

@martijnvg and I discussed this offline.

We are hesitant to merge this PR because it introduces wrong functionality.

For 8.7, we do need to fix the downstream UX effects of aggs not working (ie. errors in Lens, ML, Maps elastic/kibana#148518)

Kibana team will put a PR that sets aggregatable: false on DataViews for the time_series_type: 'counter' fields. Total impact is unclear, but it will likely end up hiding those fields in a few places (e.g. field selection in Maps/Lens/...), and disable a few other functionalities (e.g. data visualizer or Discover field-stats ). We expect that this will be an acceptable stop-gap for 8.7.

Post 8.7, we need to work on a more integrated solution. Some thoughts around this: Elasticsearch adds in field_caps which aggregations are compatible with which field-types, and Kibana acts accordingly.

cc @droberts195 @mattkime @timductive

[mattkime]

All time_series_type: 'counter' fields are aggregatable: false in this PR - elastic/kibana#150954

@jughosta Notes -

time_series_metric is not always returned via field caps:
If it’s a tsds, then yes.
If it’s just a data stream, then no => but the counter fields are still going fail for aggregations and there is no way to prevent this in Kibana. Field caps should be updated then to provide time_series_metric data for this case too.

You can quickly test this with kibana_sample_data_logs where bytes_counter has time_series_metric data in index mapping but DataViewField does not get this info.

---

I'm unclear about the usefulness of time_series_metric outside of a tsds. Perhaps they shouldn't be returned as part of the fieldcaps response.

[martijnvg]

Field caps should be updated then to provide time_series_metric data for this case too.

Ok, we can change the field caps api to always return time_series_metric property if it has been defined on a field mapping irregardless whether it is a tsdb index or data stream.

[thomasneirynck]

Ok, we can change the field caps api to always return time_series_metric property if it has been defined on a field mapping irregardless whether it is a tsdb index or data stream.

@martijnvg Are you looking into that for 8.7?

[martijnvg]

Are you looking into that for 8.7?

Yes.

[martijnvg]

Closing this PR. The conclusion is that field caps api should tell more than whether a field is aggregatable. Field caps and search APIs don't have the notion of value source types and this is an internal Elasticsearch abstraction. So it doesn't make sense to expose value source type. Maybe the field caps api should just return a list of supported aggregations per field. We will investigate this.

In the meantime Kibana will not counter fields as aggregatable.
This is the PR that fixes the mentioned issue (not returning time series attributes for fields in non tsdb indices) in field caps api: #93749