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

[APM] Serverless Onboarding with Custom Tutorials #158228

Conversation

achyutjhunjhunwala
Copy link
Contributor

@achyutjhunjhunwala achyutjhunjhunwala commented May 23, 2023

Closes #155371

Summary

PR adds Serverless Onboarding flow using Custom Integration. This would also lay the foundation for us to complete get rid of Home Tutorial App and move the remaining onPrem and cloud tutorials which are currently still loaded using Home Tutorial App.

  1. Adds new Custom Integration for Serverless Onboarding (Toggling Home AApp Tutorial Integration)

  2. Since we are migrating away from the Home App Tutorials, lot of existing code has been duplicated and refactored for the custom implementation. Home App Tutorial would require the Server to register all the steps and the client to only register a custom component which then would be loaded by Home App Tutorial component. We don't need to follow this approach any more. All the UX logic has now been moved to the Public folder with only Custom Integration done on the server/plugin.ts.

  3. As we are not sure how the solutions will be informed about being running on Serverless or not, I have introduced a new variable in serverless.oblt.yml file called xpack.apm.serverlessOnboarding: true. With this the development has been done. This can be changed to actual logic once we know more.

  4. A new configuration xpack.apm.managedServiceUrl for accessing Managed Service URL is also being added by Control Plane team as part of https://elasticco.atlassian.net/browse/CP-2403. Hence this PR expects this property to be present for Serverless.

  5. Unit tests to toggle between secret_token and api_key depending on availability has been added. No API Tests were added as no new API created. Cypress Tests cannot be added due to Serverless

Need help reviewing the PR ?

  1. config/serverless.oblt.yml - Adds the new flag which would enable this flow
  2. x-pack/plugins/apm/common/tutorial/tutorials.ts - Defines the configuration required to register the APM's Tutorial Custom Integration
  3. x-pack/plugins/apm/public/components/app/tutorials/commands - This directory contains all the agent specific data required to load the TABLE with settings required for configuring APM MIS.
  4. x-pack/plugins/apm/public/components/app/tutorials/instructions - This folder contains all the individual agent specific instructions in the format used by EuiSteps
  5. x-pack/plugins/apm/public/components/routing - Here we register our custom route
  6. Changes on the server side a quite small and they only register the custom integration.
  7. x-pack/plugins/apm/public/components/app/tutorials/serverless_instructions.tsx - This file currently defines all the logic for registering Serverless instructions. We will soon have similar files for onPrem and cloud instructions

Risk Matrix

Risk Probability Severity Mitigation/Notes
The flow depends on presence of a flag in kibana.yml file. Low High By default this flow will be disabled and would fallback to traditional onboarding in absence of the flag.

Demo

Screen.Recording.2023-05-30.at.15.48.05.mov

Updated Demo with Create API Button inside the table

Screen.Recording.2023-05-31.at.12.26.26.mov

@achyutjhunjhunwala achyutjhunjhunwala self-assigned this May 23, 2023
@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.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@achyutjhunjhunwala achyutjhunjhunwala added release_note:feature Makes this part of the condensed release notes Team:APM All issues that need APM UI Team support labels May 23, 2023
@sorenlouv
Copy link
Member

/oblt-deploy

@achyutjhunjhunwala
Copy link
Contributor Author

achyutjhunjhunwala commented Jun 6, 2023

Can you cover the tutorials with api and/or e2e tests?

@sqren Unfortunately not at this moment

API Tests - We haven't introduced any new APIs as part of it. Re-using existing one which are already covered.
E2E - We still are not clear how these tests would load for Cypress given the same challenges with Synthtrace which @kpatticha is struggling with. I believe once we have a clear path with Serverless tests or once we start adding onPrem and cloud scenarios, then it would be much easier

@sorenlouv
Copy link
Member

sorenlouv commented Jun 6, 2023

E2E - We still are not clear how these tests would load for Cypress given the same challenges with Synthtrace which @kpatticha is struggling with. I believe once we have a clear path with Serverless tests or once we start adding onPrem and cloud scenarios, then it would be much easier

Ah, bummer. Is there nothing we can do to access the new onboarding from the existing e2e setup? What if you set the needed serverless feature flag configs in the cypress config?

@achyutjhunjhunwala
Copy link
Contributor Author

@legrego Based on the discussion here, we have gated the 2 exposed configurations which were exposed to the browsers. After that i am seeing that the build has started failing. Can you have a look and help me here as to what could cause this ?

@legrego
Copy link
Member

legrego commented Jun 6, 2023

@achyutjhunjhunwala,

Based on the discussion #158228, we have gated the 2 exposed configurations which were exposed to the browsers.

Excellent, thank you!

After that i am seeing that the build has started failing. Can you have a look and help me here as to what could cause this ?

If you look at the failing test output, you'll see the following:

Error: Actual config settings exposed to the browser do not match what is expected; this assertion fails if extra settings are present and/or expected settings are missing
    at Assertion.assert (expect.js:100:11)
    at Assertion.eql (expect.js:244:8)
    at Context.<anonymous> (rendering.ts:267:37)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at Object.apply (wrap_function.js:73:16) {
  actual: '{\n' +
    '  "extra": [\n' +
    '    "xpack.apm.managedServiceUrl (any)"\n' +
    '  ]\n' +
    '  "missing": [\n' +
    '    "xpack.apm.managedServiceUrl (string)"\n' +
    '    "xpack.apm.serverlessOnboarding (boolean)"\n' +
    '  ]\n' +
    '}',
  expected: '{\n  "extra": []\n  "missing": []\n}',
  showDiff: true
}

You'll need to revert the changes made to test/plugin_functional/test_suites/core_plugins/rendering.ts to remove the config settings that are no longer exposed.

@kpatticha
Copy link
Contributor

E2E - We still are not clear how these tests would load for Cypress given the same challenges with Synthtrace which @kpatticha is struggling with. I believe once we have a clear path with Serverless tests or once we start adding onPrem and cloud scenarios, then it would be much easier

Do we need data in order to test the on boarding?

@achyutjhunjhunwala
Copy link
Contributor Author

E2E - We still are not clear how these tests would load for Cypress given the same challenges with Synthtrace which @kpatticha is struggling with. I believe once we have a clear path with Serverless tests or once we start adding onPrem and cloud scenarios, then it would be much easier

Ah, bummer. Is there nothing we can do to access the new onboarding from the existing e2e setup? What if you set the needed serverless feature flag configs in the cypress config?

That's actually an interesting point. The route is registered irrespective of serverless/non serverless mode. Its only custom integration which will not get registered. In that case, with our regular cypress suite we should be able to test it. Thank you for the hint. I will play with it 👍🏼

@sqren I am also working on creating a follow up PR where i will add the Agent Status check which we discussed here.

This PR is getting huge and i am any how creating a follow up PR immediately, is it fine to add cy test there ?

@achyutjhunjhunwala
Copy link
Contributor Author

E2E - We still are not clear how these tests would load for Cypress given the same challenges with Synthtrace which @kpatticha is struggling with. I believe once we have a clear path with Serverless tests or once we start adding onPrem and cloud scenarios, then it would be much easier

Do we need data in order to test the on boarding?

Nope, what i meant was to identify if we are in Serverless Mode for custom integration. But looks like we don't need this check

@@ -181,6 +182,7 @@ export function registerRoutes({

if (Boom.isBoom(error)) {
opts.statusCode = error.output.statusCode;
opts.body.attributes.data = error?.data;
Copy link
Member

@sorenlouv sorenlouv Jun 7, 2023

Choose a reason for hiding this comment

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

We should be careful about exposing error details (like the stack trace) to the client. I'm not sure if that's the case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a custom property where only intended data can be exposed. It will not expose stack trace. For example -

throw Boom.internal(error, { missingPrivileges }, 403);

In this case, the missingPrivileges is what will be available here.

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm. Just a few nits.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1370 1402 +32

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
apm 44 46 +2

Async chunks

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

id before after diff
apm 3.5MB 3.6MB +46.3KB
Unknown metric groups

API count

id before after diff
apm 44 46 +2

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 413 417 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 497 501 +4
total +6

History

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

cc @achyutjhunjhunwala

@achyutjhunjhunwala achyutjhunjhunwala merged commit ac2fc4c into elastic:main Jun 7, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 7, 2023
@achyutjhunjhunwala achyutjhunjhunwala deleted the serverless-onboarding-with-custom-tutorials branch June 7, 2023 14:44
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:feature Makes this part of the condensed release notes Team:APM All issues that need APM UI Team support v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Serverless onboarding