Skip to content

feat(rca): refactor item registry and manage items using api#191288

Merged
kdelemme merged 23 commits intoelastic:mainfrom
kdelemme:rca/esql-item
Aug 28, 2024
Merged

feat(rca): refactor item registry and manage items using api#191288
kdelemme merged 23 commits intoelastic:mainfrom
kdelemme:rca/esql-item

Conversation

@kdelemme
Copy link
Contributor

Resolves #191284

Summary

This PR refactors the item registry (formerly known as widget registry) in order to keep only the thing that we currently have a defined need for. When the need arise to know the schema, we will reintroduce it (needed for LLM) in the item definition.

The API for storing and retrieving investigation items is used by the investigation details page. Because investigation item types are not known in advance, the investigation item params is a Record<string, any>. The type is known when registering the item.

The current implementation for rendering the items might need some performance optimization (to be double check) in order to avoid re-rendering when adding/removing an item. At the moment, we have a simple render hook that takes the investigation items response and for each item, calls the generate() followed by the render() method using the output of the first call.
We also need to handle the time range changes, as well as sync this update with the API.

Testing

Create an investigation:

curl --request POST \
  --url http://localhost:5601/kibana/api/observability/investigations \
  --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \
  --header 'Content-Type: application/json' \
  --header 'elastic-api-version: 2023-10-31' \
  --header 'kbn-xsrf: oui' \
  --data '{
	"id": "my-awesome-id",
	"title": "my awesome investigation",
	"params": {
		"timeRange": { 
			"from": 1724260050916, "to": 1724260150916
		}
	},
	"origin": {
		"type": "blank"
	}
}'

Then you can go to the investigation details page, and add items using data-forge for faking some data.

@kdelemme kdelemme added Team:actionable-obs Formerly "obs-ux-management", responsible for SLO, o11y alerting, significant events, & synthetics. v8.16.0 labels Aug 26, 2024
@kdelemme
Copy link
Contributor Author

/ci

@obltmachine
Copy link

🤖 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!)

'@kbn/eslint/no_constructor_args_in_property_initializers': 'error',
'@kbn/eslint/no_this_in_property_initializers': 'error',
'@kbn/eslint/no_unsafe_console': 'error',
'@kbn/eslint/no_unsafe_js_yaml': 'error',
Copy link
Contributor Author

@kdelemme kdelemme Aug 26, 2024

Choose a reason for hiding this comment

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

This was making my IDE shows error in every files...
image

Copy link
Contributor

@jbudz jbudz Aug 26, 2024

Choose a reason for hiding this comment

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

Can you try resetting your environment? I'm not able to reproduce the issue.

This was a recent change - although we may be able to remove it entirely with the upgrade to js-yaml4. @legrego @elena-shostak

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did yarn kbn reset && yarn kbn bootstrap. Do you suggest another reset command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed node_modules/ and reset and now the error is gone, I'll remove this change. Sorry for the noise

@kdelemme kdelemme marked this pull request as ready for review August 26, 2024 15:57
@kdelemme kdelemme requested review from a team as code owners August 26, 2024 15:57
@elasticmachine
Copy link
Contributor

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

@kdelemme kdelemme added the release_note:skip Skip the PR/issue when compiling release notes label Aug 26, 2024
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Aug 26, 2024
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Design files changes LGTM. 1 *.scss file removed.

@mgiota mgiota self-requested a review August 26, 2024 19:14
@kibana-ci
Copy link

kibana-ci commented Aug 26, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #72 / Cloud Security Posture Test adding Cloud Security Posture Integrations CSPM AWS CIS_AWS Organization Manual Direct Access CIS_AWS Organization Manual Direct Access Workflow
  • [job] [logs] FTR Configs #86 / task_manager with mget task claimer task manager metrics task claim should increment task claim success/total counters

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
investigate 17 7 -10
investigateApp 510 509 -1
total -11

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/investigation-shared 41 44 +3
investigate 92 43 -49
total -46

Async chunks

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

id before after diff
investigateApp 348.2KB 341.7KB -6.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
investigate 5 4 -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
investigate 7.3KB 2.7KB -4.6KB
investigateApp 13.0KB 12.8KB -189.0B
total -4.8KB
Unknown metric groups

API count

id before after diff
@kbn/investigation-shared 41 44 +3
investigate 92 43 -49
total -46

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

title: suggestion.title,
type: ESQL_WIDGET_NAME,
parameters: {
type: 'esql',
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered defining the different types in constants?

});
})
);
// new
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

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.

LGTM!

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

hi all, putting this in early.

There seems to be a big overlap here with functionality on the platform-side. Can we hold off on merging before we have built more clarity?

Thank you!

@jasonrhodes
Copy link
Member

@thomasneirynck thanks for the shout-out — FWIW this is code in a plugin that isn't turned on for customers, and we're in a very exploratory phase right now, so I think we can merge this without any issues. I'll reach out and talk to you, @teresaalvarezsoler, and anyone else on your team to share more about what we're exploring and our plans for making sure we don't create weird overlap in the product. Thanks!

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

required bundles order rearrange LGTM

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Aug 28, 2024

Hi all, thanks for giving us some time to catch up on the purpose and plans.

As discussed off-thread with @jasonrhodes, @nreese, @teresaalvarezsoler

  1. We will use the next few weeks to build a better understanding of how the existing Embeddables can help with creating these types of experiences (e.g. pages of visualizations) cc @mgiota @nreese

  2. From a wider perspective, it's unclear what the role of the Dashboard app or portable Dashboards would be for the larger InspectUI effort. This will require more product validation and prototyping.

As for this PR specifically (and perhaps follow ons). It is merging into main so it feels like there is an increased risk of prototypes making it to the user. It could be worthwhile to consider different logistics, e.g. by either using a feature-branch or moving this to the examples-plugin folder. These could also be used as a temporary stopgap until we all have built more knowledge during (1).

Thank you again for the patience and understanding.

@jasonrhodes
Copy link
Member

@thomasneirynck thanks for adding the summary of our conversation here. We're fully committed to making sure that if it becomes clear that the investigations / RCA project will rely on features that Kibana platform provides, we will be sure to make use of those features rather than duplicate them. One thing I'll add to your summary is that we should follow up soon with a list of concrete gaps that we already know exist in the embeddable framework and registry, so that we can begin conversations on how those might be addressed if and when we begin to adopt these platform features. I'll try to have us put this together soon and reach out to discuss.

As for this PR and merging into main, thanks for your concerns there. We're largely happy with the process we use for prototyping new features, but the examples folder is an interesting idea that we'll look into for any new or potentially dangerous features that we may want to test in main, but that aren't as isolated from the rest of the application as this one is.

Thanks, we'll talk soon!

@kdelemme kdelemme merged commit a570d94 into elastic:main Aug 28, 2024
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Aug 28, 2024
@kdelemme kdelemme deleted the rca/esql-item branch August 28, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:actionable-obs Formerly "obs-ux-management", responsible for SLO, o11y alerting, significant events, & synthetics. v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RCA] item registry and store items on API