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

[Expressions] Introduce createTable expression function, and use in Lens #103788

Merged
merged 6 commits into from
Jul 8, 2021

Conversation

wylieconlon
Copy link
Contributor

This allows a Lens formula to contain only a static number:

Screen Shot 2021-06-29 at 5 56 49 PM

Screen Shot 2021-06-29 at 5 56 57 PM

Checklist

@wylieconlon wylieconlon added release_note:enhancement Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Team:AppServices Feature:Lens v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Jun 29, 2021
@wylieconlon wylieconlon requested a review from a team June 29, 2021 22:24
@wylieconlon wylieconlon requested a review from a team as a code owner June 29, 2021 22:24
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review for now. Just minor nitpick.

@@ -55,8 +55,8 @@ export const formulaOperation: OperationDefinition<

const visibleOperationsMap = filterByVisibleOperation(operationDefinitionMap);
const { root, error } = tryToParse(column.params.formula, visibleOperationsMap);
if (error || !root) {
return [error!.message];
if (error || typeof root === 'undefined' || root == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: root == null is sufficient for both undefined and null

@@ -35,7 +35,7 @@ function parseAndExtract(
label?: string
) {
const { root, error } = tryToParse(text, operationDefinitionMap);
if (error || !root) {
if (error || typeof root === 'undefined' || root == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM

One slight concern I have is that this strategy won't work as I would have expected if there is a bucket dimension involved, because there will be a table without any rows.

Why are there no results? Should be a constant line
Screenshot 2021-06-30 at 14 49 16

Why is the constant line stopping with the data?
Screenshot 2021-06-30 at 14 56 47

I don't think we should attempt to make these cases behave like thresholds (we will implement "real" thresholds for this very case after all), just wanted to mention it.

@wylieconlon wylieconlon mentioned this pull request Jun 30, 2021
14 tasks
@wylieconlon
Copy link
Contributor Author

@flash1293 I agree that this case is confusing. It's happening because esaggs doesn't produce any rows unless there is a metric, so the mathColumn function isn't automatically inserting a row.

I think the best option is to have a special validator that checks that the metrics are agg-based metrics, so that we can give a warning if there is a bucket agg but no metric agg.

@flash1293
Copy link
Contributor

@wylieconlon Right, we could do a warning but I'm not too worried because we don't want people to use this for thresholds anyway so it might be even good to make sure they stick with the right tool for the job.

@wylieconlon
Copy link
Contributor Author

@flash1293 After looking into this a bit more, I have decided to leave the behavior of this PR as-is in the case where you have a bucket + single number. All the current visualizations are rendering this as an empty state, which is accurate because they are missing data. I did look into adding a warning message but it would have to touch a large number of files and seems like a riskier change.

@flash1293
Copy link
Contributor

Not sure what's up with this failure - maybe it needs a bit more time to refresh the visualization?

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@wylieconlon
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
expressions 159 160 +1

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
expressions 1535 1563 +28

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.5MB 1.5MB +350.0B

Page load bundle

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

id before after diff
expressions 211.3KB 213.1KB +1.8KB
Unknown metric groups

API count

id before after diff
expressions 1966 1994 +28

History

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

@wylieconlon wylieconlon merged commit 0ef3cbe into elastic:master Jul 8, 2021
@wylieconlon wylieconlon deleted the create-table-fn branch July 8, 2021 14:02
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 8, 2021
…ens (elastic#103788)

* [Expressions] Introduce createTable expression function, and use in Lens

* Fix test

* Fix code style

* Fix typo

Co-authored-by: Kibana Machine <[email protected]>
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 8, 2021
…ens (elastic#103788)

* [Expressions] Introduce createTable expression function, and use in Lens

* Fix test

* Fix code style

* Fix typo

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.14
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 8, 2021
…ens (#103788) (#104878)

* [Expressions] Introduce createTable expression function, and use in Lens

* Fix test

* Fix code style

* Fix typo

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Wylie Conlon <[email protected]>
kibanamachine added a commit that referenced this pull request Jul 8, 2021
…ens (#103788) (#104879)

* [Expressions] Introduce createTable expression function, and use in Lens

* Fix test

* Fix code style

* Fix typo

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Wylie Conlon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants