Skip to content
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

fix(slo): Overview Embeddable drilldown actions #201870

Merged
merged 12 commits into from
Dec 3, 2024

Conversation

kdelemme
Copy link
Contributor

@kdelemme kdelemme commented Nov 26, 2024

Resolves #196740

🌸 Summary

This PR brings back the drilldown actions on the SLO Overview embeddable. I had to change the visibility of the embeddable-enhanced plugin to be able to use it from the slo plugin since these plugins don't share the same group.

In a following PR, I'll address the other embeddable components.

image

🧬 Testing

  • Create some SLO Overview embeddable (with single/grouped by SLOs)
  • Assert the drilldown action is available

@kdelemme kdelemme added release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.18.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Nov 26, 2024
@kdelemme kdelemme marked this pull request as ready for review November 26, 2024 21:12
@kdelemme kdelemme requested review from a team as code owners November 26, 2024 21:12
@elasticmachine
Copy link
Contributor

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

@mgiota
Copy link
Contributor

mgiota commented Nov 26, 2024

@kdelemme I did a quick code review and changes LGTM. I checked the visualize embeddable and the use of dynamicActionsApi that you use in your PR. I will test it out locally as well.

@Heenawter In your initial feedback you mentioned possible use of HasSupportedTriggers. I see Kevin added it as part of his PR. The visualize_embeddable doesn't use HasSupportedTriggers, but the image embeddable does. Can you explain what it does?

@mgiota mgiota self-requested a review November 26, 2024 22:17
@Heenawter
Copy link
Contributor

@mgiota I'm seeing that the visualize embeddable does use HasSupportedTriggers:

supportedTriggers: () => [
ACTION_CONVERT_TO_LENS,
APPLY_FILTER_TRIGGER,
SELECT_RANGE_TRIGGER,
],

HasSupportedTriggers is used for when you want to be able to trigger a drilldown via some other action - the image embeddable has a custom supported trigger so that clicking the image will cause the drilldown to happen, for example. Since you are returning an empty array currently, the only trigger that your drilldown can be attached to is the context menu.

Right now we are requiring supportedTriggers to be defined in order for both createDrilldownAction and editDrilldownAction to be compatible - hence, why you had to define supportedTriggers and return an empty array for those to be compatible. This is actually overkill IMO, because this is only necessary if you need other other triggers for drilldowns (other than the CONTEXT_MENU_TRIGGER) - we should probably make this an optional dependency for compatibility. cc @nreese

@mgiota
Copy link
Contributor

mgiota commented Nov 27, 2024

I'm seeing that the visualize embeddable does use HasSupportedTriggers

@Heenawter yep you are right! I initially did a quick scan for HasSupportedTriggers in the visualize_embeddable and thus my confusion. It is defined in the VisualizeApi

Thanks for the super detailed explanation! Indeed making this an optional dependency would make it simpler to use.

Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kdelemme I tested it locally and I verify it works fine!

Before
Screenshot 2024-11-27 at 10 27 16

After
Screenshot 2024-11-27 at 10 29 24

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Nov 27, 2024
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@mgiota
Copy link
Contributor

mgiota commented Nov 27, 2024

@elasticmachine merge upstream

@@ -5,7 +5,7 @@
"@elastic/kibana-presentation"
],
"group": "platform",
"visibility": "private",
"visibility": "shared",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elastic/kibana-presentation this PR opens up embeddableEnhanced so that it is used directly from solutions (in this case from o11y SLOs).

Up until now, it was only used from "platform" plugins (e.g. visualizations, dashboard_enhanced, lens, maps).

Are you okay with that transition to "shared"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this makes sense IMO. Solutions are expected to register embeddable types, and some of those will use drilldowns.

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching embeddable_enhanced to "shared" LGTM 👍

@kdelemme
Copy link
Contributor Author

kdelemme commented Dec 2, 2024

@gsoldevila Can you approve please?

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 2, 2024

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: 51c1d95
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-201870-51c1d9548354

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #89 / Dataset Quality Dataset quality handles user privileges User can read logs-* User cannot monitor any data stream Estimated data are not available due to underprivileged user

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
slo 47 48 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
slo 842.6KB 843.2KB +603.0B
Unknown metric groups

API count

id before after diff
slo 47 48 +1

History

Copy link
Contributor

@gsoldevila gsoldevila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kdelemme kdelemme merged commit 5b391b0 into elastic:main Dec 3, 2024
8 checks passed
@kdelemme kdelemme deleted the fix/embeddables-drilldown branch December 3, 2024 13:44
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

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

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- fix(slo): Access Cases from SLO details page (#201834)

Manual backport

To create the backport manually run:

node scripts/backport --pr 201870

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 4, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 201870 locally

@kdelemme
Copy link
Contributor Author

kdelemme commented Dec 4, 2024

💚 All backports created successfully

Status Branch Result
8.x

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

Questions ?

Please refer to the Backport tool documentation

kdelemme added a commit to kdelemme/kibana that referenced this pull request Dec 4, 2024
hop-dev pushed a commit to hop-dev/kibana that referenced this pull request Dec 5, 2024
@kibanamachine
Copy link
Contributor

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.

1 similar comment
@kibanamachine
Copy link
Contributor

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.

kdelemme added a commit that referenced this pull request Dec 6, 2024
)

# Backport

This will backport the following commits from `main` to `8.x`:
- [fix(slo): Overview Embeddable drilldown actions
(#201870)](#201870)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Kevin
Delemme","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-03T13:44:15Z","message":"fix(slo):
Overview Embeddable drilldown actions
(#201870)","sha":"5b391b0abe5158249ae83a393b3fdf8fdcba3c6b","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport
missing","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-management","v8.18.0"],"number":201870,"url":"https://github.com/elastic/kibana/pull/201870","mergeCommit":{"message":"fix(slo):
Overview Embeddable drilldown actions
(#201870)","sha":"5b391b0abe5158249ae83a393b3fdf8fdcba3c6b"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201870","number":201870,"mergeCommit":{"message":"fix(slo):
Overview Embeddable drilldown actions
(#201870)","sha":"5b391b0abe5158249ae83a393b3fdf8fdcba3c6b"}},{"branch":"8.x","label":"v8.18.0","labelRegex":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Shahzad <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 6, 2024
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 9, 2024
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SLO] Bring back the create drilldown menu option in SLO embeddable
7 participants