[Logs+] Extract custom integration resources to package

[Kerry350]

Summary

This closes #163788.

High level overview

• Adds a new kbn-custom-integrations package.
• This package adds a new top level custom integrations state machine, which manages a child create custom integration state machine. In the future we will have additional modes (such as adding a dataset to an existing integration, and various "uplift" flows).
• Adds connected (to the machine) components that consumers can use to facilitate custom integration workflows.
• Adds a kbn-xstate-utils package (as these utils were in 2 plugins and now 1 package).
• Replaces the integration creation inside of the onboarding wizard flow with this package.
• At the moment this is locked down to logs, and one dataset, but it can be easily extended in the future to support all types and multiple datasets. The state machine is ready, it just needs exposed in the UI.
• Some thought has gone in to how this will work with multiple "modes", and the foundations are there (imagining that certain types will be unions etc), however it's worth not getting too bogged down in those specific implementation details as I'd rather base that evolution on the real world usage when we have it.

The Configure integration section should more or less work the same as before.

Testing

• When utilising the onboarding flow for custom logs at /app/observabilityOnboarding/customLogs can you:
  • Create a custom integration? (It's worth verifying the network requests, and the assets are installed).
  • If you navigate forward, then back, make a change to the integration fields, and navigate forward again is the previously created integration deleted?
  • Is the success callout with the integration name shown on the next wizard panel?
  • Do field validations work?
  • Are errors displayed when you try to create an integration with a name that already exists?
  • Can you retry when there is a server error? (you can block network requests to the custom integrations API to test this)

Screenshots

State machine diagram

(The top level management machine is super basic, so this is just the create machine)

Followups

• Tests (the current onboarding UI implementation doesn't have tests so whilst it's not ideal technically this coverage stays the same)
• Storybook
• Replace other plugins' usage with xstate-utils (not urgent)

[ghost]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

[elasticmachine]

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

[yngrdyn]

Tests (the current onboarding UI implementation doesn't have tests so whilst it's not ideal technically this coverage stays the same)

They are coming in #165782

[Kerry350]

@tonyghiani

and I spot the following (edge) failure case when creating an integration:

Thanks. I'll get that fixed.

@yngrdyn 👌 Yep. These tests were more aimed at the state machines etc within this PR.

[Kerry350]

@tonyghiani I was thinking about the “not installed” errors a bit more for the cleanup operation. I wonder if these specific errors should actually just be silently handled and the machine allowed to progress to the success state. Ultimately if the “previously installed” item isn’t installed anyway, then what we want has already happened.

I can think of situations where it might be pertinent for the user to know that what they’ve requested to delete wasn’t found, but this is a silent cleanup 🤔

My thinking was that this option would be supplied for quicker user flows (like this wizard), but the idea is this package can fulfil all sorts of flows / implementations, so it’s entirely possible some state might be provided for something that doesn’t exist anymore.

It could also just be an option. errorOnFailedCleanup or similar, and false here 🤔

[tonyghiani]

@tonyghiani I was thinking about the “not installed” errors a bit more for the cleanup operation. I wonder if these specific errors should actually just be silently handled and the machine allowed to progress to the success state. Ultimately if the “previously installed” item isn’t installed anyway, then what we want has already happened.

Agree, this is an implicit behaviour that the user shouldn't even realize is happening as a side effect.
I would treat it as an idempotent operation, as soon as the package is deleted there is no reason to visually notify the user

I can think of situations where it might be pertinent for the user to know that what they’ve requested to delete wasn’t found, but this is a silent cleanup 🤔

Yep, we are not providing an interactive way to "remove the integration that was created", so the user shouldn't be directly informed about something that wasn't even aware was happening.

It could also just be an option. errorOnFailedCleanup or similar, and false here 🤔

Yeah is an option if we need to make it more flexible, but maybe exposing an event such as

onIntegrationCleanup: (integration: Integration, error: Error | null) => void

[weltenwort]

@Kerry350 thanks for extracting the xstate utils ❤️ I'm looking forward to v5 with the new "system" concept, which should make wiring up different statecharts even easier.

[tonyghiani]

question: does untouched represent a final state?

[Kerry350]

it's not a final state, no. I wanted something to differentiate invalid and untouched. With untouched we still want the button to be disabled (etc), due to not being valid, but we don't want to throw lots of errors at the user in the form fields because they haven't touched anything yet. Essentially the default values don't need to pass through the initial validation flow, as they're untouched. The top level UPDATE_FIELDS event will transition from here.

[tonyghiani]

suggestion: took me some time to get the logic of this validation and I noticed the cases for validatorsOrValidator being a single function or an array are handled in parallel.
We could improve its readability always converting the value into an array to have a single logical flow, wdyt?
This snippet could fit the case:

const validateConfigsAgainstContext = (validatorsConfig, context) => {
  return Object.entries(validatorsConfig).reduce((errors, [field, validatorsOrValidator]) => {
    const validators = Array.isArray(validatorsOrValidator)
      ? validatorsOrValidator
      : [validatorsOrValidator];

    const fieldErrors = validators.flatMap((validator) => {
      const error = validator(context[field]);
      return error === null ? [] : [error];
    });

    return fieldErrors.length > 0 ? { ...errors, [field]: fieldErrors } : errors;
  }, {});
};

[Kerry350]

I agree your version is more readable, but the resulting data structure is slightly different (array item errors aren’t an object keyed by array index).

I’m going to merge this PR without this change for now (just don’t want such a big PR sitting around). But I will make this more readable as part of my PR with the state machine tests 👍

[tonyghiani]

Code implementation LGTM and I appreciate the technical choice of controlling the flow with state machines!
I left a couple of notes around the PR, they are not blocking anyway.
Also thanks a lot for extracting into a package the Xstate utils, we'll be able to cleanup some existing code using it ❤️

[Kerry350]

@elastic/apm-ui Hi folks, would it be possible to get a review on this? The bulk of changes for your team should be in configure_logs.tsx. Thanks!

[yngrdyn]

this is so much cleaner now 👯

[yngrdyn]

LGTM, since you extracted some stuff to a plugin some of the observabilityOnboarding e2e tests are failing

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 6c10502

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
observabilityOnboarding	122	209	+87

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/custom-integrations	-	17	+17
@kbn/xstate-utils	-	12	+12
total			+29

Async chunks

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

id	before	after	diff
observabilityOnboarding	295.8KB	409.4KB	+113.6KB

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
@kbn/custom-integrations	-	6	+6

Page load bundle

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

id	before	after	diff
observabilityOnboarding	5.2KB	5.5KB	+338.0B

Unknown metric groups

API count

id	before	after	diff
@kbn/custom-integrations	-	19	+19
@kbn/xstate-utils	-	12	+12
total			+31

ESLint disabled in files

id	before	after	diff
@kbn/custom-integrations	-	1	+1

Total ESLint disabled count

id	before	after	diff
@kbn/custom-integrations	-	1	+1

History

• 💔 Build #158319 failed 44b28de
• 💔 Build #158150 failed d926310
• 💔 Build #157704 failed 2e0910a
• 💔 Build #157527 failed 9d05451
• 💔 Build #157175 failed 8fb83d9

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

cc @Kerry350