[Lens] Time shift metrics

[flash1293]

Fixes #68458

Adds the ability to shift individual metrics in time. The shift can either be a unit of time (1d, 6h, ...) or previous, which will shift the metric by one date bucket if there's a date histogram or by the length of the current time range if there is no date histogram in the current chart.

As long as all metrics in a chart are shifted by the same amount of time, there is no limitation in configuring it - however if there are different shifts (like no shift and a shift by one day), the terms function is not allowed. In this case an error message will the shown and the user can resolve it by pinning the current top values, creating a filters function:

Single shifted metric with terms - works fine:

Adding another unshifted metric leads to an error:

Can be "fixed" by pinning the current top values:

For the general approach check out the request/response examples in https://gist.github.com/flash1293/d9acacdd9c6439c1cf507f92b05ccca9

I’m going to add more cases to the functional testing, but the code can be reviewed already.

Code changes AggConfigs

• All metrics get a timeshift param
• Move logic to construct time filter for request into agg configs (getSearchSourceTimeFilter). If there are shifted metrics, build an OR query for all time ranges
• Add post flight transform to agg configs which is called by search source after the response is returned from ES. This is used to merge values from different time shifts (will recursively step through all bucket layers until it reaches the filters split transformTimeShift - then will merge together the splitted branches into a single one recursively mergeAggLevel) This is the core part of the logic
• Extend agg type by splitForTimeShift function which will return true if a filters split should be inserted above the dsl for this agg config
• Extend bucket agg type with new methods: getShiftedKey (to shift a bucket key, used in date histogram), orderBuckets (to order merged buckets if some only existed in one branch of the time split), getTimeShiftInterval (to get the underlying interval of the request for powering the "previous" time shift mode, used in date histogram)
• Add logic to toDsl include time filter split in the request at the right place

Code changes Lens

• Add a UI to configure time shift similar to filter UI
• Add additional validation to catch error cases
• Extend error messages with an optional fix action to provide the "Pin top values" functionality
• Add timeShift parameter to the expression ast of the datasource

Testing the agg config level (without Lens)

To test the agg config level:

• Start the functional tests server for interpreter tests (this is also what's used for automated testing): node scripts/functional_tests_server.js --config test/interpreter_functional/config.ts
• Go to http://localhost:5620
• Install some sample data
• Go to Stack management and look up id of index pattern
• Go to "Run Pipeline" app
• Open console
• Run expressions like this:

await window.runPipeline(`kibana_context timeRange={timerange from='2021-05-07T00:00:00.000Z' to='2021-05-11T00:00:00.000Z'}
                      | esaggs index={indexPatternLoad id='90943e30-9a47-11e8-b64d-95841ca0b247'}
                              aggs={aggTerms id="1" field="geo.src" size="3" enabled=true schema="bucket" orderAgg={aggCount id="order" enabled=true schema="metric"} otherBucket=true}
          aggs={aggAvg id="2" field="bytes" enabled=true schema="metric" timeShift="1d"}
          aggs={aggAvg id="3" field="bytes" enabled=true schema="metric"}`, { type: 'null' })

• Check whether the result is correct

Testing on Lens level

For each metric dimension, theres a "Shift in time" option under "Advanced options"

[flash1293]

I implemented the changes we discussed last time - with one twist: There is a case where a shifted metric works fine even with terms. That's the case if all metrics of the current layer are shifted. There is a good reason to do so:

• Have a shifted metric on the dashboard as a reference number outside of the current time range
• Have multiple layers, some shifted, shown on the same axis (without the ability to do calculations with them)

So this is what I built on this PR:

Single shifted metric with terms - works fine:

Adding another unshifted metric leads to an error:

Can be "fixed" by pinning the current top values:

As discussed I removed the warning for mismatched scaling. Thinking about it a bit more I don't even think the error message on dashboard is necessary, but I'm going to add a warning for within the editor.

What do you think @ghudgins @MichaelMarcialis @wylieconlon @dej611 @mbondyra ?

[ghudgins]

@flash1293 there's only one remedy right? If we keep the error state I would think it's because we should offer a few choices (is "go back" a reasonable second option?) .

@MichaelMarcialis depending on the answer to above, if there's only one sensible choice, should we just do the remedy (switch to Filters with "pinned" values) and offer some sort of message / toast that explains what happened? Or is that a bad UX. Just trying to avoid the "shock" of having to read an error and click a button if there's nothing else you can do. ...but also mindful that just seeing the Filters function might be equally "shocking"

[flash1293]

is "go back" a reasonable second option?)

A real go back would open another can of worms (undo/redo history), but we could offer these things as well that come close looking at the ways to get into this state in the first place:

• Remove all time shifts
• Use the same time shift for all metrics
• Remove the time shifted metric
• Remove the top values dimension

Don't really like the last two because they are potentially destructive.

[MichaelMarcialis]

@MichaelMarcialis depending on the answer to above, if there's only one sensible choice, should we just do the remedy (switch to Filters with "pinned" values) and offer some sort of message / toast that explains what happened? Or is that a bad UX. Just trying to avoid the "shock" of having to read an error and click a button if there's nothing else you can do. ...but also mindful that just seeing the Filters function might be equally "shocking"

If the resolution was a smaller or less impactful change to the chart configuration, I'd be inclined to agree with you and just automatically apply the fix. However, in this case, my first instinct is that taking control away from the user and making such a dramatic alteration to their chart configuration (i.e. changing the entire quick function) without their consent could be potentially confusing and annoying. Even if there is only one way to remedy the situation, I'd be more keen to inform the user and provide them with one or more actions they can take to either opt into a remedy or undo (via one or more of the options @flash1293 describes above) the current situation.

It would probably behoove us to involve @KOTungseth as well to finesse the wording as well, as I see we're using words like "pinning" in reference to the filters function which I'm not sure the user would understand. Also, I think tweaking the error message a bit so that it better explains to users why they can't break down by top values across two different time shifts in lay-terms would help as well.

[flash1293]

Definitely treat all copy on that PR as an initial draft :) In the past we did a pass over all new copy introduced with Kaarina or Gail close to the feature freeze.

[flash1293]

Jenkins, test this

[mbondyra]

When switching between operations, the dropdown keeps opening.

1. Add a time shift to operation.
2. move the focus to 'field' input to make sure it's not on time shift.
3. switch to another operation.
4. The dropdown opens when switching.
   Doesn't happen when closing the config panel and opening it again and then switching between operations.

Console bug with two layers - reproducible on master: #100766

When dragging a field currency to freshly converted pinned top values of category.keyword the title pinned top values of category.keyword stays

I think it shouldn't.

Filters are not being constructed properly when doing it twice

1. Configure chart as on the screenshot:

2. Drop currency to pinned top values
3. Convert it to use filters
4. The top values are being read from the previous filter created. This is what happens:

The name 'pinned top values of x'

I think this name creates another layer of having to understand what's just happened and what it is. Just converting it to filters and the name to Filters of x convinces me more.

[KOTungseth]

In my mind -0h is also a time shift, but I see how this could be confusing. Happy to follow your and @KOTungseth advice here.

Yes! I'm all for making our error messages as clear as possible. How about:

In a single layer, you are unable to combine metrics with different time shifts and dynamic top values. Use the same time shift value for all metrics, or use filters instead of top values.

[flash1293]

Thanks @mbondyra

• Changed the text as @KOTungseth suggested
• Fixed reopening dropdown
• Correctly construct filters when doing it twice
• Changed "pinned top values of x" to "Filters of x"

I didn't work on the custom label issue as it would require global changes (as discussed offline) I will handle this one as a follow-up in a separate PR.

[mbondyra]

Rechecked and everything works as expected. LGTM 👌🏼

[wylieconlon]

@flash1293 I found one bug and a few potential bugs:

1. If you type an invalid time shift, you can't delete it with backspace like a valid time shift. For example asdf can only be deleted by removing the entire time shift.

2. Whitespace isn't trimmed from time shifts, so -5d is invalid. Not a big bug, just annoying due to the above

3. When I have multiple Top Values aggs, only the first fix action is shown:

4. Left a comment about filtered metric + time offset which looks potentially concerning.

[flash1293]

Thanks for the review @wylieconlon

If you type an invalid time shift, you can't delete it with backspace like a valid time shift. For example asdf can only be deleted by removing the entire time shift.

Not handling local state correctly here, fixed that

Whitespace isn't trimmed from time shifts, so -5d is invalid. Not a big bug, just annoying due to the above

Whitespace is ignored for parsing now

When I have multiple Top Values aggs, only the first fix action is shown:

Showing all fix actions

It looks like you've added this condition in lots of places, but I'm confused: why would the presence of a filtered metric prevent time shifting? It also seems like this isn't validated in the UI:

Time shift can be used along with filtered metrics. It's omitted in the toEsAggsFn because filtered metrics get wrapped into a filtered metric pipeline agg (which will set the time shift automatically in toExpression): https://github.com/elastic/kibana/pull/98781/files#diff-724d9f5ca066cbd22bc2ab31bcd3653ea23aaa778436d23de9705e37905c9b26R125

Added a comment

The spacing of these options is also problematic.

Added spacings if there are multiple options.

[wylieconlon]

LGTM!

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: e9b556f
• Storybooks Preview

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
data	570	572	+2
lens	650	651	+1
total			+3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
data	3218	3257	+39

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lens	1.3MB	1.3MB	+19.1KB
visDefaultEditor	225.0KB	225.0KB	+35.0B
total			+19.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
data	821.8KB	839.3KB	+17.5KB
lens	34.4KB	34.6KB	+231.0B
total			+17.7KB

Unknown metric groups

API count

id	before	after	diff
data	3769	3809	+40

References to deprecated APIs

id	before	after	diff
lens	67	71	+4

History

• 💔 Build #128582 failed 77139a1
• 💚 Build #128134 succeeded 3849b04
• 💚 Build #127917 succeeded bc1633b
• 💚 Build #127829 succeeded ab828a3
• 💔 Build #127722 failed f4db9ec

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

[kibanamachine]

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 98781

[skuc-fp]

hi, in which version are you planning to release this?

[dej611]

@skuc-fp it has been merged in the 7.14 release