[Incident Management] [Suggested dashboards] Deduplicated linked dashboards from list of linked dashboards#221972

Merged

dominiqueclarke merged 13 commits intoelastic:mainfrom

dominiqueclarke:feat/alerts-related-dashboards-deduplicate-suggested

Jun 12, 2025

Contributor

dominiqueclarke commented May 29, 2025 •

edited by kibanamachine

Loading

Summary

Resolves #212801

Removes already linked dashboards from the list of suggested dashboards

Also has the side effect of returning the linked dashboards from the related dashboards api, which can be used to render the linked dashboards list along with the suggested dashboards, rather than calling a separate API from the client.


          return linked dashboards from related dashboards api

c32845b

github-actions bot added the author:obs-ux-management label

dominiqueclarke added 3 commits

June 5, 2025 10:08


          merge main

c3426de


          adjust tests

bb69bb9


          adjust deduplication logic and tests

4093ece

Contributor Author

dominiqueclarke commented Jun 5, 2025

/ci


          adjust types

Contributor Author

dominiqueclarke commented Jun 5, 2025

/ci

dominiqueclarke changed the title ~~return linked dashboards from related dashboards api~~ [Incident Management] [Suggested dashboards] Deduplicated linked dashboards from list of linked dashboards

dominiqueclarke added release_note:skip Team:actionable-obs v9.1.0 v8.19.0 backport:version labels

dominiqueclarke marked this pull request as ready for review

June 9, 2025 13:55

dominiqueclarke requested a review from a team as a code owner

June 9, 2025 13:55

Contributor

elasticmachine commented Jun 9, 2025

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)


          Merge branch 'main' into feat/alerts-related-dashboards-deduplicate-s…

ff9064e

…uggested

justinkambic reviewed

View reviewed changes

Contributor

justinkambic left a comment

A few comments regarding ways we could possibly improve error handling/logging.

Code looks good, I tested this locally with a few dashboards I made to ensure they're deduped.

...k/solutions/observability/plugins/observability/server/services/investigate_alerts_client.ts Show resolved Hide resolved

...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts Outdated

+                  const relevantDashboardsById = new Map<string, SuggestedDashboard>();
                   const index = await this.getRuleQueryIndex();
+                  if (!this.alert) {
+                    throw new Error('Alert not found. Could not fetch suggested dashboards.');

Contributor

justinkambic Jun 9, 2025

I'm noticing repetition of this throw pattern a few times throughout this class, and an inconsistent error message where sometimes the alert ID is included and other times it's not. Since alertId is a required var for this class, perhaps we can extract this throw procedure into the private function and avoid ad-hoc throw Error calls for the same purpose repeated throughout the class. It will make the error messages more standardized when researching issues if they come up as well.

Something like:

private checkAlert(): AlertData {
  if (!this.alert) throw new Error(`Alert with id ${this.alertId} not found. Could not fetch related dashboards.`);
  return this.alert;
}

...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts Outdated

+                  const allSuggestedDashboards = new Set<SuggestedDashboard>();
+                  const relevantDashboardsById = new Map<string, SuggestedDashboard>();
                   const index = await this.getRuleQueryIndex();
+                  if (!this.alert) {

Contributor

justinkambic Jun 9, 2025

Put this condition at the top of the function since it's a no-op? Also, kind of related to my comment below but also different, this exact check happens within getRuleQueryIndex, so the error thrown inside this block will never execute. This is probably fine, because the implementation of getRuleQueryIndex could change in the future. In any case, doing the throw here at the top instead of in the nested function call will make reading the stack trace easier, too.

...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts Outdated

    
                  if (allRelevantFields.length > 0) {

                    const { dashboards } = this.getDashboardsByField(allRelevantFields);

                    dashboards.forEach((dashboard) => allRelatedDashboards.add(dashboard));

                    const { dashboards } = this.getDashboardsByField(Array.from(allRelevantFields));

Contributor

justinkambic Jun 9, 2025

Suggested change

      
                  const { dashboards } = this.getDashboardsByField(Array.from(allRelevantFields));
          
                  const { dashboards } = this.getDashboardsByField(allRelevantFields);

I think we can cut this as allRelevantFields is already typed as string[].

Contributor Author

dominiqueclarke Jun 9, 2025

Interesting. Wondering if you're looking at an old commit. I don't have this on my branch.

Contributor Author

dominiqueclarke Jun 9, 2025

JK I was on my backport repo looking at this.

...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts

+                    this.getLinkedDashboards(),
+                  ]);
+                  const filteredSuggestedDashboards = suggestedDashboards.filter(
+                    (suggested) => !linkedDashboards.some((linked) => linked.id === suggested.id)

Contributor

justinkambic Jun 9, 2025

My initial impression was "why not use the set method we have below to get O(1) lookup" but I am thinking we are only going to have a handful of linked dashboards in most cases, where N=10 or less, so probably not needed. Agree? Just a thought.

...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts Outdated

+                  };
+                }
+                async fetchSuggestedDashboards(): Promise<SuggestedDashboard[]> {

Contributor

justinkambic Jun 9, 2025

Suggested change

      
              async fetchSuggestedDashboards(): Promise<SuggestedDashboard[]> {
          
              private async fetchSuggestedDashboards(): Promise<SuggestedDashboard[]> {

We could optionally encapsulate this function as we only call it internally in RelatedDashboardsClient. I do see it's referenced in the test file so if you'd rather keep it public for that purpose to avoid complicating testing I'm ok with that.

...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts Outdated

                 getRuleQueryIndex(): string | null {
                   if (!this.alert) {
-                    throw new Error('Alert not found. Could not get the rule query index.');
+                    throw new Error('Alert not found. Could not get rule query index.');

Contributor

justinkambic Jun 9, 2025

Similar to above comment about standardizing some/all of the alert-related throws. It looks like in this case there might be some difference in the information the error needs to convey, but the condition is the same.

...k/solutions/observability/plugins/observability/server/services/related_dashboards_client.ts Outdated

Comment on lines 312 to 319

+                  if (!this.alert) {
+                    throw new Error('Alert not found. Could not get linked dashboards.');
+                  }
+                  const ruleId = this.alert.getRuleId();
+                  if (!ruleId) {
+                    this.logger.warn(`Rule id not found. No linked dashboards available.`);
+                    return [];
+                  }

Contributor

justinkambic Jun 9, 2025

I've been wondering about this; is there a reason we can simply log a warning and return an empty set here while we need to throw if the alert is null?

Contributor Author

dominiqueclarke Jun 11, 2025

The rule is only necessary for linked dashboards, but technically we could still return suggested dashboards if linked dashboards fail. I don't know if that's the right UX though. We can keep it consistent by throwing the error here as well.

dominiqueclarke and others added 2 commits

June 9, 2025 14:03


          Update x-pack/solutions/observability/plugins/observability/server/se…

c3f1760

…rvices/investigate_alerts_client.ts

Co-authored-by: Justin Kambic <jk@elastic.co>


          simplify error handling

d19c66a

justinkambic approved these changes

View reviewed changes

Contributor

justinkambic left a comment

LGTM, can implement any other suggestions if you think they're worthwhile but the error throwing aspect was the only part I was truly concerned with.

dominiqueclarke added 5 commits

June 9, 2025 14:50


          adjust async action

eae132b


          adjust test

80d6afc


          adjust function accessors

f6ee6b6


          Merge branch 'main' of https://github.com/elastic/kibana into feat/al…

f2d9b5d

…erts-related-dashboards-deduplicate-suggested


          adjust types

e47fae8

Contributor

elasticmachine commented Jun 11, 2025

💚 Build Succeeded

Buildkite Build
Commit: e47fae8

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/observability-schema`	7	9	+2

Unknown metric groups

API count

id	before	after	diff
`@kbn/observability-schema`	7	9	+2

History

💔 Build #307616 failed f2d9b5d
💔 Build #306729 failed eae132b
💚 Build #306638 succeeded ff9064e
💚 Build #305989 succeeded 3295973
💔 Build #305958 failed 4093ece

dominiqueclarke merged commit 3382566 into elastic:main

10 checks passed

Contributor

kibanamachine commented Jun 12, 2025

Starting backport for target branches: 8.19

https://github.com/elastic/kibana/actions/runs/15599303794

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request


          [Incident Management] [Suggested dashboards] Deduplicated linked dash…

7f8f221

…boards from list of linked dashboards (elastic#221972)

## Summary

Resolves elastic#212801

Removes already linked dashboards from the list of suggested dashboards

Also has the side effect of returning the linked dashboards from the
related dashboards api, which can be used to render the linked
dashboards list along with the suggested dashboards, rather than calling
a separate API from the client.

---------

Co-authored-by: Justin Kambic <jk@elastic.co>
(cherry picked from commit 3382566)

kibanamachine mentioned this pull request

[8.19] [Incident Management] [Suggested dashboards] Deduplicated linked dashboards from list of linked dashboards (#221972) #223488

Merged

Contributor

kibanamachine commented Jun 12, 2025

💚 All backports created successfully

Status	Branch	Result
✅	8.19

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

This was referenced Jun 12, 2025

[dashboard] migrate embeddable_data_grid test cases to discover functional test #223452

Merged

[Incident Management] [Related dashboards api] Handle missing lens attributes gracefully #223619

Merged

[Incident Management] Render suggested dashboards #223424

Merged

kibanamachine added the backport missing label

Contributor

kibanamachine commented Jun 16, 2025

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.
cc: @dominiqueclarke

iblancof pushed a commit to iblancof/kibana that referenced this pull request


          [Incident Management] [Suggested dashboards] Deduplicated linked dash…

cf23632

…boards from list of linked dashboards (elastic#221972)

## Summary

Resolves elastic#212801

Removes already linked dashboards from the list of suggested dashboards

Also has the side effect of returning the linked dashboards from the
related dashboards api, which can be used to render the linked
dashboards list along with the suggested dashboards, rather than calling
a separate API from the client.

---------

Co-authored-by: Justin Kambic <jk@elastic.co>

Contributor

kibanamachine commented Jun 17, 2025

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.
cc: @dominiqueclarke

kibanamachine added a commit that referenced this pull request


          [8.19] [Incident Management] [Suggested dashboards] Deduplicated link…

7f569a5

…ed dashboards from list of linked dashboards (#221972) (#223488)

# Backport

This will backport the following commits from `main` to `8.19`:
- [[Incident Management] [Suggested dashboards] Deduplicated linked
dashboards from list of linked dashboards
(#221972)](#221972)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Dominique
Clarke","email":"dominique.clarke@elastic.co"},"sourceCommit":{"committedDate":"2025-06-12T01:18:22Z","message":"[Incident
Management] [Suggested dashboards] Deduplicated linked dashboards from
list of linked dashboards (#221972)\n\n## Summary\n\nResolves
https://github.com/elastic/kibana/issues/212801\n\nRemoves already
linked dashboards from the list of suggested dashboards\n\nAlso has the
side effect of returning the linked dashboards from the\nrelated
dashboards api, which can be used to render the linked\ndashboards list
along with the suggested dashboards, rather than calling\na separate API
from the client.\n\n---------\n\nCo-authored-by: Justin Kambic
<jk@elastic.co>","sha":"33825663e9118940eebc9fb6a16ec40cb9342cc7","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:obs-ux-management","backport:version","v9.1.0","v8.19.0","author:obs-ux-management"],"title":"[Incident
Management] [Suggested dashboards] Deduplicated linked dashboards from
list of linked
dashboards","number":221972,"url":"https://github.com/elastic/kibana/pull/221972","mergeCommit":{"message":"[Incident
Management] [Suggested dashboards] Deduplicated linked dashboards from
list of linked dashboards (#221972)\n\n## Summary\n\nResolves
https://github.com/elastic/kibana/issues/212801\n\nRemoves already
linked dashboards from the list of suggested dashboards\n\nAlso has the
side effect of returning the linked dashboards from the\nrelated
dashboards api, which can be used to render the linked\ndashboards list
along with the suggested dashboards, rather than calling\na separate API
from the client.\n\n---------\n\nCo-authored-by: Justin Kambic
<jk@elastic.co>","sha":"33825663e9118940eebc9fb6a16ec40cb9342cc7"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/221972","number":221972,"mergeCommit":{"message":"[Incident
Management] [Suggested dashboards] Deduplicated linked dashboards from
list of linked dashboards (#221972)\n\n## Summary\n\nResolves
https://github.com/elastic/kibana/issues/212801\n\nRemoves already
linked dashboards from the list of suggested dashboards\n\nAlso has the
side effect of returning the linked dashboards from the\nrelated
dashboards api, which can be used to render the linked\ndashboards list
along with the suggested dashboards, rather than calling\na separate API
from the client.\n\n---------\n\nCo-authored-by: Justin Kambic
<jk@elastic.co>","sha":"33825663e9118940eebc9fb6a16ec40cb9342cc7"}},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

---------

Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
Co-authored-by: Justin Kambic <jk@elastic.co>

kibanamachine removed the backport missing label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author:obs-ux-management backport:version release_note:skip Team:actionable-obs v8.19.0 v9.1.0