-
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
[Dashboard] Update Index Patterns when Child Index Patterns Change #76356
[Dashboard] Update Index Patterns when Child Index Patterns Change #76356
Conversation
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.
Copying our discussion here for discoverability:
This fix unfortunately does not work with the Lens by Value code as it is.
How I tested:
- Merge changes into the lensByValue branch.
- Create a new dashboard, see that the available filters are related to the default index pattern
- Add a lens embeddable that uses a non-default index-pattern
- See that the available filters have changed to reflect the non-default index pattern
Why doesn't this work?
In the Lens by value PR, some of the setup has been shifted from the lens embeddable factory, to the lens embeddable itself. This means that the embeddable does not know its index patterns fully at construction time. The super call looks like this now:
// in constructor
super(
initialInput,
{
editApp: 'lens',
editable: deps.editable,
},
parent
);
The index patterns are fetched later async, and updated via updateOutput:
this.updateOutput({
...this.getOutput(),
defaultTitle: this.savedVis.title,
title,
editPath: getEditPath(savedObjectId),
editUrl: this.deps.basePath.prepend(`/app/lens${getEditPath(savedObjectId)}`),
indexPatterns,
});
Because this change doesn't cause the dashboard's getOutput$
or getInput$
to emit a new value, the change is currently lost.
Next steps
We will move towards a solution similar to #76191, but hopefully the RXJS implementation can be simplified.
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.
This is definitely getting closer. It initially didn't update properly due to being one behind on the childId
s (i.e. not reflecting the most recent change) I fixed this by switching the subscription to getOutput$()
instead of getInput$()
.
Even after that change, it appeared not to be picking up output updates on children between dashboard output updates - which is where the lens embeddable now adds its indexPatterns.
I was able to fix that, by switching out merge
for combineLatest
. After both changes, the dashboard picks up on indexPattern changes as expected. Here is the outputSubscription as it looks currently.
outputSubscription = merge(
// output of dashboard container itself
dashboardContainer.getOutput$(),
// plus output of dashboard container children,
// children may change, so make sure we subscribe/unsubscribe with switchMap
dashboardContainer.getOutput$().pipe(
map(() => dashboardContainer!.getChildIds()),
distinctUntilChanged(deepEqual),
switchMap((newChildIds: string[]) =>
combineLatest(
newChildIds.map((childId) => dashboardContainer!.getChild(childId).getOutput$())
)
)
)
)
This is simpler than my original implementation, but I would still appreciate a once-over. Is combineLatest the right choice here? Why doesn't merge behave as expected in this case? I will do a little bit more research
Thanks again for doing this @Dosant
@ThomThomson, besides |
This is on purpose, |
@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.
Tested in Chrome, works as expected!
Good catch with the merge / combineLatest syntax confusion, and this is a much simpler solution overall.
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
page load bundle size
History
To update your PR or re-run it, just comment with: |
I will add a functional test to the LensByValue PR to make sure that the index patterns are reflected correctly. This test won't rely on the feature flag because it will work the same way regardless of 'by value' or 'by reference' |
…lastic#76356) Added the ability for Dashboard to react to index pattern changes in its children's output.
…76356) (#76807) Added the ability for Dashboard to react to index pattern changes in its children's output. Co-authored-by: Anton Dosov <[email protected]>
* master: (47 commits) Do not require id & description when creating a logstash pipeline (elastic#76616) Remove commented src/core/tsconfig file (elastic#76792) Replaced whitelistedHosts with allowedHosts in actions ascii docs (elastic#76731) [Dashboard First] Genericize Attribute Service (elastic#76057) [ci-metrics] unify distributable file count metrics (elastic#76448) [Security Solution][Detections] Handle conflicts on alert status update (elastic#75492) [eslint] convert to @typescript-eslint/no-unused-expressions (elastic#76471) [DOCS] Add default time range filter to advanced settings (elastic#76414) [Security Solution] Refactor NetworkTopNFlow to use Search Strategy (elastic#76249) [Dashboard] Update Index Patterns when Child Index Patterns Change (elastic#76356) [ML] Add option to Advanced Settings to set default time range filter for AD jobs (elastic#76347) Add CSM app to CODEOWNERS (elastic#76793) [Security Solution][Exceptions] - Updates exception item find sort field (elastic#76685) [Security Solution][Detections][Tech Debt] - Move to using common io-ts types (elastic#75009) [Lens] Drag dimension to replace (elastic#75895) URI encode the index names we fetch in the fetchIndices lib function. (elastic#76584) [Security Solution] Resolver retrieve entity id of documents without field mapped (elastic#76562) [Ingest Manager] validate agent route using AJV instead kbn-config-schema (elastic#76546) Updated non-dev usages of node-forge (elastic#76699) [Ingest Pipelines] Processor forms for processors K-S (elastic#75638) ...
Closes #76171
(alternative to #76191)
Summary
Makes sure dashboard app controller updates the list of index patterns any time the indexPatterns key in the output of a child embeddable changes. This allows embeddables to lazily load the list of index patterns instead of requiring them upfront.
Skipping the test for this change, because dashboard_app_controller code is not testable (no units there 😢 ) and from functional test perspective this can be only covered in future lensByValue pr. @ThomThomson, maybe this is something you could try to cover in your future functional test?