-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[TSVB] Filter by clicking on the timeseries chart #97426
Conversation
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Pinging @elastic/kibana-app (Team:KibanaApp) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found some nits but overall it's working great
id, | ||
name: layer.metrics[0].field || seriesPerLayer[0].splitByLabel, | ||
isMetric: true, | ||
type: layer.metrics[0].type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this is necessary? As there can be multiple metrics and the first one might not even be used, I'm not sure whether it would work in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right, I changed it to the latest metric in the list. If I don't miss anything this is always the active one per layer on TSVB.
eb75702
const table = tables[termArray[0]]; | ||
|
||
const layer = model.series.filter(({ id }) => id === termArray[0]); | ||
let splitLabel = termArray.length > 1 ? termArray[1] : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can you extract termArray.length > 1
into a constant like hasSplit
or something similar? I tripped over this a few times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to const [layerId, splitLabel] = specId.split(':');
and used the splitLabel instead. Are you ok with it?
} | ||
|
||
export const addMetaToColumns = ( | ||
columns: TSVBColumns[], | ||
indexPattern: IndexPattern, | ||
metricsType: string | ||
indexPattern: IndexPattern | ||
): DatatableColumn[] => { | ||
return columns.map((column) => { | ||
const field = indexPattern.getFieldByName(column.name); | ||
const type = (field?.spec.type as DatatableColumnType) || 'number'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether it matters, but this isn't correct for filters - it will assume type as number
because there is no field in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flash1293 this is now the case on viz, here is the example of the meta on a filters column (taken by xy axis)
type === number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, guess we should fix this on esaggs side as well. Anyway, for consistency reasons it makes sense to keep it for now 👍
}; | ||
if (column.type === BUCKET_TYPES.FILTERS && column.params) { | ||
const filters = column.params.map((col) => ({ | ||
input: col.filter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think if you add label: col.label
in here, it will pick it up as custom label for the filter pill (this is what happens for Lens/Visualize as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed eb75702
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works as expected - LGTM
* WIP TSVB filter by click * Disable filter click when showBar is set to false * Exclude metric columns as they are not filtered * Allow filters group by filter click event * Fix CI and unit tests * Add some comments * Move to separate function for easier testing * Add more unit tests * Add a functional test * Improve types * Fix bug with group by filters and user applies custom labels * fix time filter bug * Address PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* WIP TSVB filter by click * Disable filter click when showBar is set to false * Exclude metric columns as they are not filtered * Allow filters group by filter click event * Fix CI and unit tests * Add some comments * Move to separate function for easier testing * Add more unit tests * Add a functional test * Improve types * Fix bug with group by filters and user applies custom labels * fix time filter bug * Address PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Closes #83892
Adds support to create filters by clicking on a TSVB timeseries chart when is embedded on a dashboard. When the user edits/creates the visualization and clicks the chart, no filter is applied as the filter bar is not rendered.
As a bonus, the URL and dashboard drilldowns for click events are now supported 🎉
Filter by click on a dashboard:
Dasboard drilldowns - click event:
Checklist
Delete any items that are not applicable to this PR.