Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] Enable new context constants in formula #158452

Merged
merged 23 commits into from
Jun 6, 2023
Merged

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented May 25, 2023

Summary

Fixes #151827, Fixes #117548

This PR contains constant function features in Lens formula:

Screenshot 2023-05-29 at 12 05 43 Screenshot 2023-05-29 at 12 04 18
  • and now() (as suggested in here) functions in the formula input
Screenshot 2023-05-25 at 14 30 14

A visual explanation of what each constant represent:
Screenshot 2023-05-29 at 11 56 16

Documentation:

Screenshot 2023-05-31 at 12 59 40

Some example usage
Screenshot 2023-05-25 at 13 33 26

TSVB => Lens with params._interval support:
tsvb_to_lens_with_interval

Notes:

  • context values work like static values, with the same limits, therefore it is not possible to build a date histogram with a context value alone (i.e. a formula with only interval() or now()). It works ok without the Date Histogram dimension.
  • The interval() function will report an error if used without a configured Date Histogram dimension:
Screenshot 2023-05-29 at 12 14 13
  • The interval() function does not take into account different bucket interval size (i.e. DST changes, leap years, etc...), rather return the same value to all the buckets. This is the same behaviour as in TSVB, but in Lens it can be a problem due to the usage of calendar_interval.

  • I had to duplicate a couple of function from the helpers to avoid issues with tests. I've tried a different organization of the helpers (between pure vs impure fns) but that took longer than expected, so I would defer this task later in another PR.

General approach with `constant(...)` removed * a more general approach using `constants(value="...")` Screenshot 2023-05-29 at 12 07 23

A cloud deployment has been created to test both approaches here. Let me know which one do you prefer cc @elastic/kibana-visualizations

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens ci:cloud-deploy Create or update a Cloud deployment labels May 25, 2023
@dej611 dej611 marked this pull request as ready for review May 31, 2023 16:27
@dej611 dej611 requested a review from a team as a code owner May 31, 2023 16:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it!! I have some questions:

  1. Why the timerange() gives me B in the new metric/
image
  1. Why this returns no data?
image
  1. I am not sure about the kibana context title here. It makes sense to me but does it make sense for the users/
    @gchaps @KOTungseth can you help here?
image

@dej611
Copy link
Contributor Author

dej611 commented Jun 1, 2023

I love it!! I have some questions:

  1. Why the timerange() gives me B in the new metric/
image

I see you've taken a ~15 days time range, so in ms number values it's about 1.3 billion ms:

moment.duration(1300000000).humanize()
> '15 days'

moment.duration('15', 'd').asMilliseconds()
> 1296000000 // ~1.3b
  1. Why this returns no data?
image

That is because it's subject to the same logic as any static value, unfortunately, over a date histogram: same applies if you write 5.
We thought in the past to handle this specific case applying a + count() - count() special formula under the hood, but never got the time to do it - nor think it's a good practice anyway 😅 .

  1. I am not sure about the kibana context title here. It makes sense to me but does it make sense for the users/
    @gchaps @KOTungseth can you help here?
image

Yes, a contribution on their side would be great here 👍

@stratoula
Copy link
Contributor

I see you've taken a ~15 days time range, so in ms number values it's about 1.3 billion ms:

Oh it is a billion not bytes 😆

defaultMessage: 'Kibana context',
}),
i18n.translate('xpack.lens.formulaDocumentation.constantsSectionDescription', {
defaultMessage: 'Functions used to retrieve Kibana context variables.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other Formula sections in this window include thoroughly broken down definitions.

How about:
These functions are used to retrieve Kibana context variables, which are... and help you to....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've revisited the copy here. Maybe we could discuss a bit and have a consistent copy for the entire section with a follow up PR here?

label: 'Date histogram interval',
description: i18n.translate('xpack.lens.indexPattern.interval.documentation.markdown', {
defaultMessage: `
The current date histogram interval bucket expressed in milliseconds (ms).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

The specified minimum interval for the date histogram, in milliseconds (ms).

label: 'Current now',
description: i18n.translate('xpack.lens.indexPattern.now.documentation.markdown', {
defaultMessage: `
The current now moment used in Kibana expressed in milliseconds (ms).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure what now moment means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the instant value of now, expressed in ms

label: 'Time range',
description: i18n.translate('xpack.lens.indexPattern.timeRange.documentation.markdown', {
defaultMessage: `
The current time range expressed in milliseconds (ms).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

The specified time range, in milliseconds (ms).

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I am not sure about the now description but I don't have a better alternative tbh.

image

Other than that (we can always improve on a follow up PR) everything seems to work fine. LGTM!

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I am not sure about the now description but I don't have a better alternative tbh.

image

Other than that (we can always improve on a follow up PR) everything seems to work fine. LGTM!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jun 6, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #64 / machine learning - short tests model management trained models for ML power user displays a model with an ingest pipeline and model can be deleted with associated ingest pipelines

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 884 885 +1

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.2MB 1.2MB +3.1KB
visTypeTimeseries 507.6KB 507.7KB +82.0B
total +3.2KB

Page load bundle

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

id before after diff
lens 34.9KB 34.9KB +31.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 413 417 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 497 501 +4
total +6

History

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

@dej611 dej611 merged commit 3600f97 into elastic:main Jun 6, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 6, 2023
@dej611 dej611 removed the ci:cloud-deploy Create or update a Cloud deployment label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Support bucket interval on formulas Timespan user variable
6 participants