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

[Logs+] Create an integration while on-boarding logs #163219

Merged
merged 17 commits into from
Aug 11, 2023

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Aug 4, 2023

Summary

This closes #161960, a basic integration will now be created whilst onboarding logs (though the custom logs flow).

This implements the initial version of this work, and does not include things like adding a dataset to an existing integration.

UI / UX

General:

Screenshot 2023-08-07 at 15 20 21

Naming conflict errors:

Screenshot 2023-08-11 at 13 34 45

Screenshot 2023-08-11 at 13 34 59

Lack of permissions error:

Screenshot 2023-08-09 at 17 10 35

General errors:

Screenshot 2023-08-07 at 16 49 40

Success callout on the next panel:

Screenshot 2023-08-07 at 17 20 45

Delete previous flow (happens in the background):

delete_process

Pointers for reviewers / next steps

  • This PR also creates a new package for the useTrackedPromise hook, as this is used in several places and I didn't want to just duplicate it again (I haven't replaced other current uses in this PR, but will as a followup).

  • useFetcher was avoided as A) it's very tightly coupled with the observability onboarding server route repository (and callApi is scoped to this) and I wanted to call an "external" API in Fleet and B) I wanted explicit control over when the request is dispatched (not on mount), and whilst this can sort of be achieved by not returning a promise from the callback it gets quite messy. I also wanted more granular error handling control.

  • Moving forward I think we'll need to enhance the state management of the plugin. We'll want to add the ability to "add to existing integration" and this is going to make the state more complex (even with chunks of this functionality likely moved to it's own package). I did actually have the Wizard state moved in to a constate container at one point (as a starter) but I reverted this commit to make the changeset less intrusive. It's for this same reason that, for now, I haven't focussed too closely on extracting things like generating the friendly error messages etc as we'll likely want to extract some of the "create integration" hooks / UI in to a standalone package so they can be used elsewhere (not just onboarding). There are also quite a few eslint-disable-next-line react-hooks/exhaustive-deps rules in the plugin at the moment due to the references not being stable, we could improve that at the same time as any state changes.

  • You can technically navigate directly to /fox/app/observabilityOnboarding/customLogs/installElasticAgent, but no state is stored in the URL, so nothing is rehydrated resulting in a very empty configuration. I'm not entirely sure this is a behaviour we want, but for now I've just made the callout conditional on state existing (so coming from the previous panel).

  • The Fleet custom integrations API now throws a 409 (conflict) when using a name that already exists.

Testing

  • Head to /app/observabilityOnboarding to trigger the onboarding flow
  • Select "Stream log files"
  • When hitting "continue" an integration should be created in the background (check the network requests for api/fleet/epm/custom_integrations)
  • When continuing (to install shipper), then going back and making changes to your integration options, when clicking continue again there should be a network request that deletes the previously created integration (to clean things up). This should be seamless to the user.
  • You should not be able to use a name that already exists (for an existing custom integration)
  • General errors (like permission issues, asset installation issues) should display at the bottom
  • When you hit the next panel (install shipper) there should be a success callout that also contains the name of the integration that was created

In progress

Two changes still in progress, but they don't need to hold up the review (8.10 coming soon 👀):

  • To have a friendlier error for permissions issues (not just "forbidden")
  • Fleet API integration test for the naming collision

@apmmachine
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.
  • /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!)

@Kerry350 Kerry350 force-pushed the 161960-onboarding-integration-creation branch 2 times, most recently from 0b9b881 to 4a6e396 Compare August 7, 2023 10:54
@Kerry350 Kerry350 force-pushed the 161960-onboarding-integration-creation branch from e4416ee to dc77da0 Compare August 8, 2023 11:20
@Kerry350 Kerry350 self-assigned this Aug 8, 2023
@Kerry350 Kerry350 added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes labels Aug 8, 2023
@Kerry350 Kerry350 marked this pull request as ready for review August 8, 2023 14:17
@Kerry350 Kerry350 requested review from a team as code owners August 8, 2023 14:17
@elasticmachine
Copy link
Contributor

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

@Kerry350 Kerry350 requested a review from tonyghiani August 8, 2023 14:18
@tonyghiani
Copy link
Contributor

tonyghiani commented Aug 9, 2023

I made a first feature review before checking the code:
✔️ When hitting "continue" an integration should be created in the background.
✔️ When continuing (to install shipper), then going back and making changes to your integration options, when clicking continue again there should be a network request that deletes the previously created integration (to clean things up). This should be seamless for the user.
✔️ You should not be able to use a name that already exists (for an existing custom integration)
✔️ General errors (like permission issues, asset installation issues) should display at the bottom
✔️ When you hit the next panel (install shipper) there should be a success callout that also contains the name of the integration that was created

I gladly notice that manually visiting the log explorer profile after the onboarding, the integration is correctly listed with its dataset in the Integrations list 👏

The only thing I noticed (non-blocking at first) is that we have nice error handling when there is a conflicting name, but it doesn't handle capitalized names in the same name, there should be a payload validation in the endpoint that returns a 400 bad request if the integration name has capitalized letters and report the error under the input field as we do for a conflicting name.

Overall looks smooth, I'll get into the code for the next review round!

UPDATE: I just noticed that using the name of an existing non-custom integration doesn't generate a naming conflict, and overrides the existing installed integration.
e.g. system integration
Before:

Screen.Recording.2023-08-09.at.09.01.41.mov

Create custom integration with the conflicting name:
Screenshot 2023-08-09 at 09 02 24

After:

Screen.Recording.2023-08-09.at.09.02.38.mov

I think we should validate the naming conflict also against the installed packages which are not of type custom, does it sound right?

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Aug 9, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@yngrdyn
Copy link
Contributor

yngrdyn commented Aug 9, 2023

When installing system integration (for system logs onboarding) we are showing this to the user
image

Maybe it's worthy to align in that matter, though not a blocker for this PR.

@Kerry350
Copy link
Contributor Author

Kerry350 commented Aug 9, 2023

@yngrdyn Thanks for the heads-up, I'll match the messaging.

Copy link
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

Code changes LGTM!

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observabilityOnboarding 109 114 +5

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/use-tracked-promise - 2 +2

Async chunks

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

id before after diff
observabilityOnboarding 277.9KB 285.9KB +7.9KB

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/use-tracked-promise - 1 +1
Unknown metric groups

API count

id before after diff
@kbn/use-tracked-promise - 3 +3

ESLint disabled in files

id before after diff
@kbn/use-tracked-promise - 1 +1

ESLint disabled line counts

id before after diff
@kbn/use-tracked-promise - 1 +1

Total ESLint disabled count

id before after diff
@kbn/use-tracked-promise - 2 +2

History

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

cc @Kerry350

@Kerry350 Kerry350 requested a review from juliaElastic August 11, 2023 14:23
Copy link
Contributor

@juliaElastic juliaElastic left a 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 changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs+ Onboarding] Create an integration while on-boarding application logs
8 participants