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

[Fleet] Add custom integrations API #112481

Merged

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Sep 16, 2021

Part of #93084


Summary

internal changes:

  • adds new plugin custom_integrations. This plugin can be used by others to register integrations that need to show up on the fleet-integration UX app
  • register tutorials inside home plugin
  • typing changes. Some types are moving into custom_integrations. Others became more generic to accomodate non-epr integration-types.

user-facing changes:

  • adds new categories and cards to the fleet-integration app. Since it's only registering tutorials, this is just the EMS-card for now. The APM card tutorial remains hidden. APM will need similar replacement behavior, just like Beats

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@thomasneirynck thomasneirynck added chore release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.16.0 v8.0.0 labels Sep 21, 2021
@thomasneirynck thomasneirynck changed the title [Fleet] Add custom categories [Fleet] Add custom integrations Sep 21, 2021
@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

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

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Did a first pass look at the basic structure and I generally think the approach makes sense. This will definitely be cleaner once we can move the merging logic to the server-side and provide a single API endpoint that returns the merged and normalized data.

@thomasneirynck thomasneirynck marked this pull request as ready for review September 22, 2021 15:23
@thomasneirynck thomasneirynck requested review from a team as code owners September 22, 2021 15:23
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Took a look here and went through @joshdover's previous comments. I don't have anything much to add of concern other than what he already mentioned and what was resolved there.

I think the typing is a bit of a lingering concern, but I'm comfortable with what we have set up for IntegrationCard objects, etc here in this PR.

I pulled this PR down locally and verified the functionality of the EMS tutorial integration card. Things look good!

Thanks for all your work on this! 🚀

@@ -0,0 +1,9 @@
# customIntegrations
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs will need fleshing out and conversion to .mdx if we want to include them in the new dev docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx. my preference is to hold off on dev-docs in this PR. These will evolve fairly quickly. Would prefer to introduce only in separate PR when contract stabilizes.

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Some NITs and comments, overall LGTM

src/plugins/home/kibana.json Outdated Show resolved Hide resolved
src/plugins/home/server/plugin.ts Outdated Show resolved Hide resolved
src/plugins/home/server/plugin.ts Outdated Show resolved Hide resolved
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.

The approach looks ok to me although the new custom_integrations plugin is sorely missing unit tests.

I know it seems like a harmless enough plugin but we need to make sure it gives new users a great first impression!

IMO, we should also add support for localization as soon as possible. 7.16 will be around for a while.

src/plugins/custom_integrations/common/index.ts Outdated Show resolved Hide resolved
src/plugins/home/server/plugin.ts Outdated Show resolved Hide resolved
src/plugins/home/server/plugin.ts Outdated Show resolved Hide resolved
@TinaHeiligers TinaHeiligers self-requested a review September 23, 2021 23:58
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.

LGTM

@thomasneirynck
Copy link
Contributor Author

thomasneirynck commented Sep 27, 2021

@TinaHeiligers thx for the review! I added unit test coverage and a new api integration test for the new plugin.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
customIntegrations - 6 +6
fleet 569 574 +5
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
customIntegrations - 66 +66
fleet 1085 1095 +10
total +76

Async chunks

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

id before after diff
fleet 595.4KB 596.7KB +1.3KB

Page load bundle

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

id before after diff
customIntegrations - 2.4KB +2.4KB
fleet 130.7KB 132.4KB +1.7KB
total +4.1KB
Unknown metric groups

API count

id before after diff
customIntegrations - 66 +66
fleet 1186 1196 +10
total +76

History

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

@thomasneirynck thomasneirynck merged commit be1ee57 into elastic:master Sep 27, 2021
@thomasneirynck thomasneirynck added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 27, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 112481

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Sep 27, 2021
Add a new plugin `custom_integrations`. This plugin allows for the registration of data-integrations tutorials. The Fleet-integrations app will display these alongside the existing Elastic Agent integrations.
# Conflicts:
#	packages/kbn-optimizer/limits.yml
thomasneirynck added a commit that referenced this pull request Sep 27, 2021
Add a new plugin `custom_integrations`. This plugin allows for the registration of data-integrations tutorials. The Fleet-integrations app will display these alongside the existing Elastic Agent integrations.
# Conflicts:
#	packages/kbn-optimizer/limits.yml
@dikshachauhan-qasource
Copy link

Hi @thomasneirynck

As the merges are now available for above PR, is it possible that we can have mocks for user-facing changes.

Also, it will be great if you could provide us more info on how to validate above.

Currently, attempted to look out changes on latest 8.0 snapshot build available on cloud-qa platform, however couldn't find as described in ticket.

image

Further, we observed an inactive link for custom inputs under integration tab. So we have reported issue [8.0]: Custom inputs link under integration tab is inactive..

cc @kpollich @EricDavisX

Thanks
QAS

@EricDavisX
Copy link
Contributor

@dikshachauhan-qasource since it is closed, I would also follow up with an email, to the author after a day if there is no response. it would be good to confirm if we have mocks, but we may not. :) thanks for testing and poking it.

@thomasneirynck
Copy link
Contributor Author

@dikshachauhan-qasource these are primarly internal changes. The only user-facing change is the additiong of the EMS-tutorial as a new card.

So from the pointers at the screenshot here, #112481 (comment), that's not related to this change. Doing a custom-search, should behave exactly as on 7.15. If there are bugs there (e.g. inactive link). Can you log as a new issue? thx!

@dikshachauhan-qasource
Copy link

Hi @thomasneirynck

Thanks for the feedback.

We have validated above related issue on shipped 7.15 build and found it working fine.

Further, as per ticket, Please assign all the related tickets that will be coming under this feature change, so that we can record them and validate at our end.

Thanks
QAS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.