-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Incident Management] When fetching dashboards referenced panels are also fetched #228811
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
Conversation
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
b797833 to
9f47052
Compare
...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts
Outdated
Show resolved
Hide resolved
73b4582 to
e4ba2e7
Compare
dominiqueclarke
left a comment
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'd ideally like to fetch each panel reference only once.
Currently, if two or more dashboards share the same reference, the reference is fetched multiple times. Tested by saving a relevant lens visualization to the library, and then adding it to multiple dashboards.
Also, I noticed that relevant panels are not considered when added to a dashboard via the "Copy to Dashboard" control. When adding a relevant panel to a different dashboard using the control, the dashboard I copied over to was surfaced as a suggested dashboard

...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts
Show resolved
Hide resolved
...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/observability/server/services/helpers.ts
Show resolved
Hide resolved
...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts
Outdated
Show resolved
Hide resolved
nickpeihl
left a comment
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.
presentation team changes lgtm
code review only
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Unknown metric groupsAPI count
History
|
...ck/solutions/observability/plugins/observability/server/services/referenced_panel_manager.ts
Outdated
Show resolved
Hide resolved
...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts
Outdated
Show resolved
Hide resolved
|
|
||
| import type { DashboardAttributes, DashboardPanel } from '../server/content_management'; | ||
|
|
||
| export const isDashboardPanel = ( |
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.
Is isDashboardPanel needed? Why not just use existing isDashboardSection?
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.
TypeScript doesn’t automatically infer that if a value is not a DashboardSection, then it must be a DashboardPanel. To avoid manual type casting, I created a second type guard function.
| // So, if the same saved object panel is referenced in two different dashboards, it will have different panelIndex values in each dashboard, but the same panelId, since they're both referencing the same panel. | ||
| private panelsById = new Map<string, ReferencedPanelAttributesWithReferences>(); | ||
| private panelIndexToId = new Map<string, string>(); | ||
| private panelsTypeById = new Map<string, string>(); |
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 looks great, but I'm just confused a bit again by the variable names. Maybe panelsToFetchById. Maybe not the best. I just don't understand what 'type' means here semantically.
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.
The key of the map stores the panel ID and the value stores the panel type, I'll need both to fetch the panels.
That's why I called this map panelsTypeById, it follows the valueBykey pattern and reflects what’s stored.
What is you find confusing?
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.
Thanks. I haven't looked at this domain in a while so I've been getting confused about what each individual piece is and how they all work together
After stepping through the code some more, I see the type is essentially the saved object type. Thank you very much for explaining.
dominiqueclarke
left a comment
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.
LGTM! Thanks for the work.
This PR closes #212125
Acceptance criteria
This evaluation of by reference visualizations should happen in the same related dashboards API request (not a separate one) ✅