[APM] NP Migration - Moves plugin server files out of legacy by ogupte · Pull Request #57532 · elastic/kibana

ogupte · 2020-02-13T03:23:44Z

Closes #56830
Move server files of the APM plugin from x-pack/legacy/plugins/apm -> x-pack/plugins/apm. This still initializes legacy plugin, but it allows us to very easily switch over to the new platform when we fully migrate the client app.

Also Closes #56832
Migrates uses of the saved objects client API from the legacy references to those exposed on the CoreStart object. It removes the requirement to pass in a saved objects client to the getApmIndices() function exposed on the APM plugin.

ogupte · 2020-02-13T08:58:43Z

x-pack/legacy/plugins/infra/server/routes/metadata/lib/has_apm_data.ts

getApmIndices() no longer requires a saved objects client to be passed in.

ogupte · 2020-02-13T09:01:52Z

x-pack/plugins/apm/kibana.json

added usageCollection plugin as an optional dependency for service telemetry

ogupte · 2020-02-13T09:13:37Z

x-pack/plugins/apm/server/lib/apm_telemetry/index.ts

uses the savedObjectsClient directly rather than obtaining a new instance from the legacy kibana server

ogupte · 2020-02-13T09:15:31Z

x-pack/plugins/apm/server/lib/helpers/get_internal_saved_objects_client.ts

smaller helper function to get the internal saved objects client from the core setup object since we do this in a few places.

ogupte · 2020-02-13T09:18:33Z

x-pack/plugins/apm/server/lib/index_pattern/create_static_index_pattern.ts

I changed this to a scoped saved object client, which used to be internal before. i tested it with the apm read user and it was able to persist the index pattern for discover/kuery bar/etc.

ogupte · 2020-02-13T09:20:40Z

x-pack/plugins/apm/server/lib/settings/apm_indices/save_apm_indices.ts

the reference to Object.fromEntries produced a typescript error (maybe it's not fully supported?) so i just converted it to a reduce.

ogupte · 2020-02-13T09:24:09Z

x-pack/plugins/apm/server/plugin.ts

the usage collector is now registered from the plugin setup, but it can't block setup since it depends on core start services, so it uses regular old Promise API.

ogupte · 2020-02-13T09:28:29Z

x-pack/plugins/apm/server/routes/services.ts

it was necessary to add a catch here since the CI test saw an unresolved promise when there was a failure to persist telemetry attributes and failed CI.

@watson I recall that you added this check (throw on unresolved promise) to the test runner. Did you also enable it at runtime in dev mode?

when i ran the api integration tests locally, the same unresolved promise error shows in the server log, but the test runner did not mark it as a failure

I was just thinking whether this also would throw during normal development and not just when running tests

Yes, the check was added in #51431.

For CI the magic is here:

kibana/src/dev/ci_setup/setup_env.sh

Line 17 in 5bb2481

export NODE_OPTIONS="$NODE_OPTIONS --throw-deprecation"

In dev-mode (when running yarn start), the magic is here:

kibana/package.json

Line 56 in 5bb2481

"start": "node --trace-warnings --throw-deprecation scripts/kibana --dev",

I intended it to run during dev-mode, so that developers would not be surprised by weird errors in CI that they might as well catch early on in development.

Note that if you manually run node scripts/kibana it will not throw on deprecations.

Btw, normally I would think that you should pass the error on to the UI so that the user could be notified that something went wrong instead of just logging it to STDERR. But I'm not sure what this code does and if it makes sense to do one or the other in this case. Just be sure you thought it through 🙂

normally I would think that you should pass the error on to the UI so that the user could be notified that something went wrong instead of just logging it to STDERR.

Agree! However, the operation in this case is a background job which is implicitly initiated by the user but the response of the API doesn't rely on it.

@ogupte I think we talked about adding a comment to make this more obvious

ogupte · 2020-02-13T09:29:45Z

x-pack/plugins/apm/typings/common.d.ts

not sure how this was valid before

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/Popover/ServiceMetricList.tsx

sorenlouv · 2020-02-14T12:39:41Z

x-pack/plugins/apm/common/projections/errors.ts

Why is it forbidden to import from the server folder?

Actually, I think the linter has a point: range_filter should be moved to common if it's consumed by something from common.

sorenlouv · 2020-02-14T12:41:43Z

x-pack/plugins/apm/server/lib/index_pattern/create_static_index_pattern.ts

Looks much better 👍

sorenlouv · 2020-02-14T13:01:29Z

x-pack/plugins/apm/server/lib/metrics/by_agent/java/gc/get_gc_time_chart.ts

Thanks for fixing the casing everywhere 👍

sorenlouv · 2020-02-14T13:55:03Z

x-pack/plugins/apm/server/lib/helpers/get_internal_saved_objects_client.ts

No biggie but any reason not to async/await like everywhere else?

const [coreStart] = await core.getStartServices() return coreStart.savedObjects.createInternalRepository();

kindsun · 2020-02-18T20:04:23Z

Removed myself from the reviewers list since it looks like this doesn't touch the Maps app. Feel free to re-add me if I'm mistaken!

Kerry350

Infra changes LGTM 👍

and context scoped clients exposed in the new platform core setup

…ndices, and uses the internal saved objects client when creating apm index pattern, so that any user who navigates to apm can trigger it

…e_casing

kibanamachine · 2020-02-19T20:05:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b150999

History

💚 Build #27532 succeeded 7074e7bc4cee933741f6cc7cff3b33e4dbed290e
💚 Build #27414 succeeded d6b631497aba7b04300e35277a9cbaee95bbc459
💔 Build #26705 failed 3ec54f25816cd27373ff69efd3167042bfea44b1
💔 Build #26433 failed 669834986bf0994921f7f510480bc5c911fa195a
💔 Build #26412 failed 0b888d4064f1f47795a6c59a37c3157496e9413d

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

ogupte · 2020-02-19T21:42:55Z

cla/trigger

…#57532) * Closes @56832 Migrates uses of the saved objects client to the internal and context scoped clients exposed in the new platform core setup * moves apm server, common, and typings dirs to the new plugin directory * fixes path imports and type errors * fixes some lint errors * fixes CI failure. Use internal saved objects client like before. * uses the context-scoped saved objects client for saving runtime APM indices, and uses the internal saved objects client when creating apm index pattern, so that any user who navigates to apm can trigger it * fixes the type check error by updating import paths * renamed files and directories to snake_case to pass scripts/check_file_casing * rebase fixes and commit filename case changes * moves get_indices_privileges.ts out of legacy path

…#58074) * Closes @56832 Migrates uses of the saved objects client to the internal and context scoped clients exposed in the new platform core setup * moves apm server, common, and typings dirs to the new plugin directory * fixes path imports and type errors * fixes some lint errors * fixes CI failure. Use internal saved objects client like before. * uses the context-scoped saved objects client for saving runtime APM indices, and uses the internal saved objects client when creating apm index pattern, so that any user who navigates to apm can trigger it * fixes the type check error by updating import paths * renamed files and directories to snake_case to pass scripts/check_file_casing * rebase fixes and commit filename case changes * moves get_indices_privileges.ts out of legacy path

* master: (136 commits) [Visualize] Remove legacy appState in visualize (elastic#57330) Use static time for tsvb rollup test (elastic#57701) [SIEM] Fix ResizeObserver polyfill (elastic#58046) [SIEM][Detection Engine] Fixes return codes where some were rule_id instead of id skip flaky suite (elastic#56816) skip flaky suite (elastic#58059) skip flaky suite (elastic#45348) migrates notification server routes to NP (elastic#57906) Moved all of the show/hide toggles outside of ordered lists. (elastic#57163) [APM] NP Migration - Moves plugin server files out of legacy (elastic#57532) [Maps][Telemetry] Migrate Maps telemetry to NP (elastic#55055) Embeddable add panel examples (elastic#57319) Fix useRequest to support query change (elastic#57723) Allow custom paths in plugin generator (elastic#57766) [SIEM][Case] Merge header components (elastic#57816) [ML] New Platform server shim: update job audit messages routes (elastic#57925) [kbn/optimizer] emit success event from reducer when all bundles cached (elastic#57945) [APM] Don’t include UI filters when fetching a specific transaction (elastic#57934) Upgrade yargs (elastic#57720) skip flaky suite (elastic#57762) (elastic#57997) (elastic#57998) ... # Conflicts: # src/plugins/advanced_settings/public/management_app/components/field/__snapshots__/field.test.tsx.snap # src/plugins/advanced_settings/public/management_app/components/field/field.tsx # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

Caused by: elastic/kibana#57532

ogupte added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Feb 13, 2020

ogupte requested a review from a team February 13, 2020 03:23

ogupte requested a review from a team as a code owner February 13, 2020 03:23

ogupte force-pushed the apm-56830-np-migration-plugin-server branch from 0b888d4 to 6698349 Compare February 13, 2020 08:39

ogupte mentioned this pull request Feb 13, 2020

[APM] NP Migration - Saved Objects Client #57438

Closed

ogupte commented Feb 13, 2020

View reviewed changes

x-pack/plugins/apm/kibana.json Outdated

Copy link

Contributor Author

ogupte Feb 13, 2020

Choose a reason for hiding this comment

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

added usageCollection plugin as an optional dependency for service telemetry

ogupte commented Feb 13, 2020

View reviewed changes

x-pack/plugins/apm/typings/common.d.ts Outdated

Copy link

Contributor Author

ogupte Feb 13, 2020

Choose a reason for hiding this comment

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

not sure how this was valid before

ogupte force-pushed the apm-56830-np-migration-plugin-server branch from 6698349 to 3ec54f2 Compare February 13, 2020 23:59

sorenlouv reviewed Feb 14, 2020

View reviewed changes

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/Popover/ServiceMetricList.tsx Outdated Show resolved Hide resolved

sorenlouv reviewed Feb 14, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/index_pattern/create_static_index_pattern.ts Outdated

Copy link

Member

sorenlouv Feb 14, 2020

Choose a reason for hiding this comment

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

Looks much better 👍

sorenlouv reviewed Feb 14, 2020

View reviewed changes

x-pack/plugins/apm/server/lib/metrics/by_agent/java/gc/get_gc_time_chart.ts Outdated

Copy link

Member

sorenlouv Feb 14, 2020

Choose a reason for hiding this comment

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

Thanks for fixing the casing everywhere 👍

sorenlouv reviewed Feb 14, 2020

View reviewed changes

sorenlouv approved these changes Feb 17, 2020

View reviewed changes

ogupte force-pushed the apm-56830-np-migration-plugin-server branch from fdcdb8d to d6b6314 Compare February 18, 2020 17:57

spalger requested a review from a team as a code owner February 18, 2020 17:57

spalger requested a review from a team February 18, 2020 17:57

spalger requested review from a team as code owners February 18, 2020 17:57

spalger requested review from kindsun and removed request for a team and kindsun February 18, 2020 18:14

kindsun removed their request for review February 18, 2020 20:03

ogupte force-pushed the apm-56830-np-migration-plugin-server branch from d6b6314 to 7074e7b Compare February 19, 2020 01:00

Kerry350 approved these changes Feb 19, 2020

View reviewed changes

ogupte added 10 commits February 19, 2020 08:52

Closes @56832 Migrates uses of the saved objects client to the internal

853ce80

and context scoped clients exposed in the new platform core setup

moves apm server, common, and typings dirs to the new plugin directory

249e3d6

fixes path imports and type errors

4371bc6

fixes some lint errors

e67cbb6

fixes CI failure. Use internal saved objects client like before.

1cd3e86

uses the context-scoped saved objects client for saving runtime APM i…

7cb66cb

…ndices, and uses the internal saved objects client when creating apm index pattern, so that any user who navigates to apm can trigger it

fixes the type check error by updating import paths

2040fc0

renamed files and directories to snake_case to pass scripts/check_fil…

e4edd25

…e_casing

rebase fixes and commit filename case changes

c32e1ed

moves get_indices_privileges.ts out of legacy path

b150999

ogupte force-pushed the apm-56830-np-migration-plugin-server branch from 7074e7b to b150999 Compare February 19, 2020 18:01

ogupte merged commit 96a0aa0 into elastic:master Feb 19, 2020

ogupte mentioned this pull request Feb 20, 2020

[7.x] [APM] NP Migration - Moves plugin server files out of legacy (#57532) #58074

Merged

v1v added a commit to v1v/apm-integration-testing that referenced this pull request Feb 21, 2020

apm-ui: location was changed

c5810ff

Caused by: elastic/kibana#57532

This was referenced Feb 21, 2020

apm-ui: use the new kibana project layout elastic/apm-integration-testing#769

Merged

[apm-ui] Tests are broken since a recent change in the Kibana apm-ui layout elastic/apm-integration-testing#770

Closed

cauemarcondes added the apm-test-plan-7.7.0 label Mar 30, 2020

Conversation

ogupte commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogupte Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kindsun commented Feb 18, 2020

Uh oh!

Kerry350 left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 19, 2020

💚 Build Succeeded

History

Uh oh!

ogupte commented Feb 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ogupte commented Feb 13, 2020 •

edited

Loading

ogupte Feb 13, 2020 •

edited

Loading

sorenlouv Feb 14, 2020 •

edited

Loading