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] Extend bounds for Interval operation #132619

Closed
wants to merge 9 commits into from

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented May 20, 2022

Summary

Fixes #129308

This PR introduces the ability to extend the data bounds for the interval operation ( numeric histogram ) in a similar way to Visualize.

By default it is disabled:

Screenshot 2022-05-20 at 14 49 19

When enabled a new range input is shown, with current data bounds already filled in and some explanation about lack of support of filtering:

Screenshot 2022-05-20 at 14 45 07

The field has some basic validation:

Screenshot 2022-05-20 at 14 45 17

When Include empty rows is disabled the feature is disabled:

Screenshot 2022-05-20 at 14 45 24

In case of no data available, then the field is shown with empty values:

Screenshot 2022-05-20 at 14 52 07

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 v8.3.0 labels May 20, 2022
@@ -624,6 +624,7 @@ export function LayerPanel(
render={layerDatasource.renderDimensionEditor}
nativeProps={{
...layerDatasourceConfigProps,
activeData: props.framePublicAPI.activeData,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs this bit here to prepopulate the bounds field on first enabling.

layer={state.layers[layerId]}
layerId={layerId}
activeData={props.activeData}
{...paramEditorProps}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While debugging some missing props, I've noticed this duplication here. I can be moved into a separate PR, but it's well isolated already.

[maxBarsValue]
[onMaxBarsChange]
);
const { inputValue: maxBarsValue, handleInputChange: setMaxBarsValue } =
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 taken the opportunity here to refactor this with the useDebounceValue hook.

rows: [],
formattedColumns: {},
};
for (const row of table.rows) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

while testing the formatting solution, I've noticed here some time iterating on the formattedRows at each execution. With this mutable version it should save some time for big tables.

);
// This special function will override partial rows value to empty strings
// to make it work for the renderer.
const partialRowsOverride: ExpressionAstFunction[] = shouldEnablePartialRows
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, this solution seems the "cleanest" so far: the table values gets overridden only in some specific cases and the rest of the logic remains the same.
It might be arguable that the final result produced makes the empty values for the partial rows fall into the (empty) category.

@dej611
Copy link
Contributor Author

dej611 commented May 26, 2022

Notes for reviewers

This feature is enabled on the datasource side, so it will show up in all charts that support the Interval bucketing function. Nonetheless some chart types will ignore the partial rows anyway, like the partition ones.
I've thought about hide this option for specific chart types that do not support it, but that would be inconsistent with the rest of the UI. Im open to suggestion on this side.

@dej611 dej611 added release_note:enhancement backport:skip This commit does not require backporting auto-backport Deprecated - use backport:version if exact versions are needed v8.4.0 and removed backport:skip This commit does not require backporting labels May 26, 2022
@dej611 dej611 marked this pull request as ready for review May 26, 2022 12:34
@dej611 dej611 requested a review from a team as a code owner May 26, 2022 12:34
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@dej611 dej611 removed the v8.3.0 label May 26, 2022
@flash1293 flash1293 added backport:skip This commit does not require backporting and removed auto-backport Deprecated - use backport:version if exact versions are needed labels May 30, 2022
@flash1293
Copy link
Contributor

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

I'm not sure about the solution with partial rows - filling in empty strings causes weird behavior:
Screenshot 2022-05-30 at 17 30 02

  • empty is filtered out, but it's still there? (because the partial rows got an empty string value)
  • it has the value 0 (I guess because the metric also got an empty string value which is casted to 0 somewhere (Number('') === 0)

We could introduce some super special __only_here_as_a_placeholder__ value instead of empty string which is known to the renderers and not shown in the chart but taken into account for the extents, but that seems like overkill.

Maybe we should just disallow having a number histogram with extended bounds as the parent agg of a terms agg? It's also a little weird (user might wonder why this limitation), but it seems less weird than the empty/0 issue. I see how it would be a valid use case, the user would have to approximate in this case by manually defining each individual range (it seems OK for me because that's an edge case)

What do you think @dej611 @ghudgins @markov00 ?

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 870 873 +3

Async chunks

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

id before after diff
expressionXY 86.7KB 86.7KB -12.0B
lens 1.2MB 1.2MB +2.9KB
total +2.9KB

Page load bundle

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

id before after diff
lens 32.4KB 32.8KB +407.0B
Unknown metric groups

async chunk count

id before after diff
lens 13 14 +1

History

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

@markov00
Copy link
Member

@flash1293 I've hard time following what is happening, can we discuss this online and find a solution?

@flash1293
Copy link
Contributor

flash1293 commented May 31, 2022

Another approach would be to pass the meta information of the chosen domain to the chart directly instead of inferring it from the data table. This would require us to implement this logic per visualization.

@dej611
Copy link
Contributor Author

dej611 commented Jun 1, 2022

We could introduce some super special __only_here_as_a_placeholder__ value instead of empty string which is known to the renderers and not shown in the chart but taken into account for the extents, but that seems like overkill.

It is an option to evaluate to be honest.

Maybe we should just disallow having a number histogram with extended bounds as the parent agg of a terms agg? It's also a little weird (user might wonder why this limitation), but it seems less weird than the empty/0 issue. I see how it would be a valid use case, the user would have to approximate in this case by manually defining each individual range (it seems OK for me because that's an edge case)

I think the general idea of disabling the feature in some specific case can be a possible solution: there are also other scenarios, like with partition charts where this feature does not bring any value and it does not make much sense to show it.

Another approach would be to pass the meta information of the chosen domain to the chart directly instead of inferring it from the data table. This would require us to implement this logic per visualization.

Probably the best solution here: the meta information can be optional and when this feature is enabled it is passed over.

@flash1293
Copy link
Contributor

like with partition charts where this feature does not bring any value and it does not make much sense to show it

It's a good point, that brings us back to implementing this as part of the individual chart types instead of data modelling (sorry for pushing you in the wrong direction with this initially)

@dej611
Copy link
Contributor Author

dej611 commented Jun 9, 2022

Closing in favour of #134020

@dej611 dej611 closed this Jun 9, 2022
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.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Extended bounds for number histogram
6 participants