[Lens] Introduce 4 new calculation functions: counter rate, cumulative sum, differences, and moving average #84384
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f0fba99 to
808a0d0
Compare
|
@elasticmachine merge upstream |
|
merge conflict between base and head |
flash1293
reviewed
Dec 14, 2020
There was a problem hiding this comment.
I noticed a few things, not all of them caused by this PR:
- Cumulative sum will not fill the entire time range if there are gaps in the data
-
It's possible to set the window of moving average to 0 or negative values
-
The dimension is not shown in error state on incomplete configuration (like it's used for missing fields):
- If the sub-operation of "Differences" is changed after it was valid, it ends up in a weird state where it seems to work, but the inputs are still shown in error state - Configure "Differences of average of bytes", then change "average" to "maximum":
- It seems weird "advanced options" are shown above the window parameter (also, the spacing is not quite right):
-
Are all visualizations using references filtered out? This seems a little excessive, I think we should at least keep those not changing the data table at all. Switching between charts is "unwrapping" the reference (e.g. from bar chart to table, turning counter rate into max) - this is definitely not what I would expect as a user, will this be fixed on the transition PR?
-
If there is no date histogram, the dimension is shown in error state, but the label is not centered:
-
It's possible to select Counter rate / Cumulative Sum and so on even if there is no date histogram. Maybe I'm missing something, I was under the impression we were going to disable the option in the operation picker in this case with an explanatory tooltip
-
Suffix formatter is not playing well with missing values
- Value formatter doesn't seem to have any effect
| singleSelection={{ asPlainText: true }} | ||
| onChange={(choices) => { | ||
| if (choices.length === 0) { | ||
| // onDeleteColumn(); |
There was a problem hiding this comment.
nit: replaced by the updateLayer call?
This was referenced Dec 15, 2020
|
@flash1293 I have some partial responses here, categorized first as things that I don't think I caused, and then more detailed about things I can work on: Things I don't think I caused:
Things that I can work on:
This was intentional because I wanted to discuss. We usually delete the incomplete config instead of showing a warning, and I think that it might be a more correct UX if we don't ever save incomplete configs. What do you think?
Will fix.
Will fix by moving the time scaling underneath the ParamEditor.
I started work on this in this PR and have some tests for this, so I think I'll fix in this PR
This was pretty useful for testing edge cases, I'm fixing now though. |
I'm not sure I understand 100% - in my screenshot I closed the flyout, so it's "saved" in the editor state, right? I think we should show it as a proper error because you also can't go back to the previous state of the dimension easily. Like the following situation:
|
|
The changes you made look pretty good to me, suggestions and calculation-retaining chart switches are missing but that's probably intentional.
Just checked and Visualize is handling it in the same way, please disregard.
This is just a small layout thing - the text in the dimension is not vertically centered. Most likely a general problem |
flash1293
reviewed
Dec 16, 2020
| return checkForDateHistogram( | ||
| layer, | ||
| i18n.translate('xpack.lens.indexPattern.movingAverage', { | ||
| defaultMessage: 'Moving average', |
There was a problem hiding this comment.
Those messages all reference moving average, but I guess they are meant to reference the respective calculation (in this case "Differences", same in cumulative sum and so on)
|
Found one more thing in the behavior: On deleting the field out of the sub function for moving average, the sub function itself is deleted as well. I think we could keep it in this case:
In the other direction, the same thing happens. Also, in this scenario the label is not reset and still references the old configuration. Do we even need the ability to delete the function/field? It seems like there is no valid situation where that would be required. I think we can just make the comboboxes non-deletable and force the user to have something chosen there. |
|
I agree that we should save the work in progress state here. Updated all the error messages in the latest commit.
Opened an EUI bug for this elastic/eui#4394
Fixed this bug though by adding a label update |
|
You are probably still busy with this - AFAICT the following things are still open
|
|
@flash1293 The functionality is working the way I expect it to, but I had to do a refactoring to support the "last value" GUI inside references. I'll put up the refactoring commit as a separate PR tomorrow, and focus on some smokescreen testing. |
|
The state on this PR looks pretty good, the only thing still left I can see here is "Showing an incomplete dimension using the error state (red text and warning icon)" - we can also discuss this offline if you want to. I reviewed the refactoring commit you mentioned splitting out into a separate PR and I'm not sure I see why this refactoring is necessary. It's adding the operation param UI to the nested editor (this part is definitely necessary, good catch!), but do we need to do the refactoring switching from |
1 task
|
We talked about showing a currently selected, but invalid operation in an error state, do you plan to do it on this PR or will we tackle it later? In this screenshot "Cumulative sum" is selected, but there is no date histogram: As soon as I hover, it changes like this |
|
Calculation operations are also shown in the side bar if there is no timefield in the current index, making it impossible to ever add a calculation. Not sure how important, but wanted to point it out. We do handle it for the regular operations (e.g. average, min, max is hidden if there are no number fields) |
flash1293
reviewed
Dec 21, 2020
| const referenceColumn = layer.columns[referenceId]!; | ||
| const requirements = | ||
| // @ts-expect-error not statically analyzed | ||
| operationDefinitionMap[column.operationType].requiredReferences[index]; |
There was a problem hiding this comment.
@ts-expect-error can make refactorings harder in the future, what about this approach?
const definition = operationDefinitionMap[column.operationType];
if (definition.input !== 'fullReference') {
throw new Error('inconsistent state - column is not a reference operation');
}
const requirements = definition.requiredReferences[index];|
As discussed offline, there are a few way how we could treat invalid state (parts may happen on this PR):
All of them have some kind of underlying logic |
|
Minor issues while manual testing it:
|
|
You mean for the second part they should behave like other metrics @dej611 ? If you select a string field on cumulative sum, it will just switch away to unique count? |
dej611
reviewed
Dec 21, 2020
Comment on lines
114
to
134
| const functionOptions: Array<EuiComboBoxOptionOption<OperationType>> = []; | ||
| operationSupportMatrix.operationTypes.forEach((operationType) => { | ||
| const def = operationDefinitionMap[operationType]; | ||
| const label = operationPanels[operationType].displayName; | ||
| const isCompatible = | ||
| !column || | ||
| (column && | ||
| hasField(column) && | ||
| def.input === 'field' && | ||
| operationSupportMatrix.fieldByOperation[operationType]?.has(column.sourceField)) || | ||
| (column && !hasField(column) && def.input !== 'field'); | ||
|
|
||
| functionOptions.push({ | ||
| label, | ||
| value: operationType, | ||
| className: 'lnsIndexPatternDimensionEditor__operation', | ||
| 'data-test-subj': `lns-indexPatternDimension-${operationType}${ | ||
| isCompatible ? '' : ' incompatible' | ||
| }`, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
nitpick: this looks like a map operation:
const functionOptions: Array<EuiComboBoxOptionOption<OperationType>> = operationSupportMatrix.operationTypes.map((operationType) => {
...
return {
label,
value: operationType,
className: 'lnsIndexPatternDimensionEditor__operation',
'data-test-subj': `lns-indexPatternDimension-${operationType}${
isCompatible ? '' : ' incompatible'
}`,
}
});There was a problem hiding this comment.
You're right that it's basically a map, but we can't do Set.map so I will do Array.from(set).map instead
Comment on lines
27
to
29
| i18n.translate('xpack.lens.indexPattern.missingFieldLabel', { | ||
| defaultMessage: 'Missing field', | ||
| }), |
There was a problem hiding this comment.
This looks like a frequent flyer: can it be moved into a shared util?
| @@ -151,6 +150,8 @@ export function insertNewColumn({ | |||
| columnId | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
Noticed this duplicated code also in a previous PR. Just a refactor idea.
Given that it is not returning in branches anymore, what do you think of conditionally assign the function and then have a generic flow?
const addOperationFn = isBucketed ? addBucket : addMetric;
return updateDefaultLabels(
addOperationFn(
tempLayer,
operationDefinition.buildColumn({
...baseOptions,
layer: tempLayer,
referenceIds,
}),
columnId
)
);To me it has the benefit to streamline also the branching that follows below
| columnOrder: getColumnOrder({ ...tempLayer, columns: newColumns }), | ||
| columns: adjustColumnReferencesForChangedColumn(newColumns, columnId), | ||
| }; | ||
| // const newColumns = { ...tempLayer.columns, [columnId]: newColumn }; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The issue I see is that supported fields gets "muted": in the picture all the shown fields should have a solid colouring because they are all supported by
|
|
@dej611 you're right that it looks strange when in the field-only editor that is used by cumulative sum. The same behavior makes sense when looking at the editor for moving average. I'll see what I can do. |
|
@dej611 @flash1293 thanks for the feedback. I've fixed almost all the issues you've described, but I did not come up with a solution to show the chart without errors while configuring a new reference. I think that can be added in a follow-up PR. Done in the latest:
|
flash1293
approved these changes
Dec 22, 2020
wylieconlon
pushed a commit
to wylieconlon/kibana
that referenced
this pull request
Dec 22, 2020
…e sum, differences, and moving average (elastic#84384) * [Lens] UI for reference-based functions * Fix tests * Add a few unit tests for reference editor * Respond to review comments * Update error handling * Update suggestion logic to work with errors and refs * Support ParamEditor in references to fix Last Value: refactoring as needed * Fix error states * Update logic for showing references in dimension editor, add tests * Fix tests Co-authored-by: Joe Reuter <johannes.reuter@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
wylieconlon
pushed a commit
that referenced
this pull request
Dec 22, 2020
…e sum, differences, and moving average (#84384) (#86786) * [Lens] UI for reference-based functions * Fix tests * Add a few unit tests for reference editor * Respond to review comments * Update error handling * Update suggestion logic to work with errors and refs * Support ParamEditor in references to fix Last Value: refactoring as needed * Fix error states * Update logic for showing references in dimension editor, add tests * Fix tests Co-authored-by: Joe Reuter <johannes.reuter@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Joe Reuter <johannes.reuter@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
This was referenced Jan 4, 2021
💔 Build Failed
Failed CI Steps
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/api_keys/home_page·ts.API Keys app Home page Loads the appStandard OutStack TraceChrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/home_page·ts.Actions and Triggers app Home page Alerts tab navigates to an alert details pageStandard OutStack TraceChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/logstash/pipeline_list·js.logstash pipeline list route add button links to the empty pipeline editorStandard OutStack Traceand 5 more failures, only showing the first 3. Metrics [docs]Module Count
Async chunks
Distributable file count
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Release Notes: Adds counter rate, cumulative sum, differences and moving average to Lens
Introduces the following 4 functions to the Lens UI, with new error handling:
In this PR, your field selection is cleared every time you switch between these functions. This will be fixed in a later PR because of the added complexity.
This PR also changes the behavior when fields go missing or have changed type in the mappings. On master, these messages are collapsed into one line. In this PR each message gets its own line.
Error states
Incomplete configuration:
Missing date histogram:
Unfinished field selection:
Errors with multiple layers:
Checklist
Delete any items that are not applicable to this PR.