reverts #150954

[ppisljar]

Summary

reverts #150954

Time Series Metric Counter field is aggregatable and should be reported as such. Field is however different from a normal number field as it doesn't support all aggregations the normal number field does. This is similar to rolled up number fields.
Editors should take this into account to make the best possible experience for the user.

since reverting the original PR the following editors have been updated to support this kind of fields:

• lens
• discover (field statistics view)
• maps
• ML

[stratoula]

@thomasneirynck this PR reverts the changes that Matt had done on 8.7 to mark this type of field as not aggregatable. Are we ok to merge it now? Lens is going to support these fields. Maps and canvas(?) should possibly do some changes (either support it or hide it). cc @elastic/kibana-presentation

[thomasneirynck]

Editors should take this into account

The problem was that having this flagged as aggregatable was causing a toast storm in places like ML data visualizer in 8.7, silent errors in Discover, and visible errors in Lens/Maps. (#148518). I understand this trade-off was accepted, but I believe only in the context of Lens, where the error is "opt-in" (since user needs to explicitly select the field). In other apps, like ML, they show up point-blank.

A robust path forward post 8.7 is for field-caps to indicate exactly what aggregations are supported/not-supported.(elastic/elasticsearch#93539 (comment)) This will be more effort on the client-side, because it will have to accomodate this new metadata and use it accordingly (e.g. Lens disabling certain aggs/enabling others).

Already the problem has been mitigated somewhat for 8.7, removing the counter field-type from non-tsdb indices (elastic/elasticsearch#93749 (comment)).

We do need to move forward, but I don't think we can just re-enable, as it will reintroduce the same errors on the ML-side for tsdb-stream. cc @peteharverson @droberts195 If we just re-enable this, we would need to a solution for ML.

[ppisljar]

regarding field caps providing info on what aggregations are supported:

• apps would still need to check this and behave correctly (not much different as us having logic on kibana side to make this decisions)
• just information on what aggs can be run directly on the field is not enough. There are restrictions to how you can connect the aggregations together as well. For example rate aggregation can run on the counter metric field, but only when its inside timeseries aggregation. And timeseries aggregation can only be executed inside date_histogram aggregation.
  This kind of information will not be part of the field caps response imo. In fact i think if every field on field caps response will contain big list of supported aggs that will be a lot of duplicated info on that respoonse.
• @martijnvg is what you are talking about in this comment Certain Aggregation functions not supported when TSDB datastream is used. elasticsearch#93539 (comment) referring to all fields on field caps?

[ppisljar]

Regarding counter fields on non-tsdb indices:

• does that make sense ? and will it ever be supported ? we definetely didnt account for that
• i can see where the confusion comes, logs sample dataset was updated so its indexed as a datastream. Some TSDB properties were also added to the fields. But the datastream is not marked as TSDB. @martijnvg i was expecting that if i try to add field mapping to non TSDB dataset with some TSDB specific properties either those properties will be ignored (and the field will be a normal number) or elasticsearch will error out with error complaining that i am trying to set invalid setting. What is the actual behaviour around this ?

[martijnvg]

For example rate aggregation can run on the counter metric field, but only when its inside timeseries aggregation. And timeseries aggregation can only be executed inside date_histogram aggregation.

I don't think this is true. The rate aggregation can run on other number fields as well, but the rate computation will be different (less correct) and no resets can be detected. The time_series aggregation can be used outside of date_historgram as well. For example as a top level aggregation or as part of a terms aggregation.

is what you are talking about in this comment elastic/elasticsearch#93539 (comment) referring to all fields on field caps?

Yes, and it is going to look like this: elastic/elasticsearch#93884
Each field in field caps is going to include what aggregations are supported.

i was expecting that if i try to add field mapping to non TSDB dataset with some TSDB specific properties either those properties will be ignored (and the field will be a normal number) or elasticsearch will error out with error complaining that i am trying to set invalid setting. What is the actual behaviour around this ?

The behaviour is that time series attributes in the mapping will be ignored on non tsdb data streams.

[stratoula]

@thomasneirynck it seems that the issue that we encountered in 8.7 is not going to happen now. @ppisljar did some tests on what is going to be affected and these are:

• maps (it fields dropdown, needs to limit to only certain aggs (just like in lens))
• canvas, visualize, tsvb, vega
• discover field statistics view
• ML "visualize data from dataview" (if you select dataview that is tsdb and has counter fields ML will show errors for those).

They can possibly filterout these fields.

[droberts195]

What does the "Field Statistics" tab in Discover look like for the logs sample data after these changes?

This is what it looks like before these changes:

What does it look like with these changes?

It's a really bad user experience if Discover doesn't work for one of the sample data sets that comes with Kibana. That is the sort of thing that could put a first time user off from continuing their trial into a second day.

The idea of that view is to give users a vague idea of what sorts of values are in each field in their data. It's not designed to be mega accurate, just some eye candy really. We'll need to decide what to show for counter fields in the "Distributions" column using one of the aggregations that works.

[ppisljar]

for logs sample dataset it works as expected.

for datasets with time series mode on and counter metric field it errors out

[ppisljar]

this PR addresses the issue in ML and discover: #152898

[ppisljar]

this PR addresses the issue in Maps application: #152899

[thomasneirynck]

I created an issue to summarize the problem here #152912. It seems there are basically two trains of thought:

• each apps handles counter-field individually (basically hiding the field when aggs are being run by default)
• Elasticsearch publishes metadata, which can then be used by clients to fine-tune UX

thx @ppisljar for making an explicit suggestion on how this could be addressed without using the new proposed field_caps metadata.

[jgowdyelastic]

this PR addresses the issue in ML and discover: #152898

It's worth noting that PR #152898 does not fix all issues in ML.
I've raised a separate PR to fix the errors thrown in our anomaly detection and data frame analytics wizards.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: e3adf41

Failed CI Steps

• FTR Configs #52
• FTR Configs #56

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
securitySolution	433	436	+3

Total ESLint disabled count

id	before	after	diff
securitySolution	513	516	+3

History

• 💔 Build #114931 failed fc129cb
• 💔 Build #114895 failed 3475ea6
• 💚 Build #114287 succeeded bdef8e6
• 💔 Build #113964 failed 2e8f116
• 💚 Build #112070 succeeded efafba4

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[mattkime]

changes lgtm, nice work