[APM] NP Migration - Moves plugin server files out of legacy #57532

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

-  const apmIndices = await framework.plugins.apm.getApmIndices(
-    requestContext.core.savedObjects.client
-  );
+  const apmIndices = await framework.plugins.apm.getApmIndices();


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

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

x-pack/plugins/apm/kibana.json

@@ -6,5 +6,5 @@
  "configPath": ["xpack", "apm"],
  "ui": false,
  "requiredPlugins": ["apm_oss", "data", "home"],
-  "optionalPlugins": ["cloud"]
+  "optionalPlugins": ["cloud", "usageCollection"]


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

@@ -26,34 +25,28 @@ export function createApmTelementry(
 }

 export async function storeApmServicesTelemetry(
-  server: APMLegacyServer,
+  savedObjectsClient: InternalSavedObjectsClient,


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

@@ -0,0 +1,16 @@
+/*


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

-      context.__LEGACY.server
-    );
-    await internalSavedObjectsClient.create(
+    const savedObjectsClient = context.core.savedObjects.client;


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

@@ -27,9 +27,8 @@ export async function saveApmIndices(

 // remove empty/undefined values
 function removeEmpty(apmIndices: Partial<ApmIndicesConfig>) {
-  return Object.fromEntries(


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

      })
    );

+    const usageCollection = plugins.usageCollection;


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

+    const savedObjectsClient = await getInternalSavedObjectsClient(core);
+    storeApmServicesTelemetry(savedObjectsClient, apmTelemetry).catch(error => {
+      context.logger.error(error.message);
+    });


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

@@ -7,7 +7,7 @@
 import '../../infra/types/rison_node';
 import '../../infra/types/eui';
 // EUIBasicTable
-import {} from '../../reporting/public/components/report_listing';
+import '../../reporting/public/components/report_listing';


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

 } from '../../server/lib/helpers/setup_request';
 import {
  PROCESSOR_EVENT,
  SERVICE_NAME,
  ERROR_GROUP_ID
 } from '../elasticsearch_fieldnames';
+// eslint-disable-next-line @kbn/eslint/no-restricted-paths
 import { rangeFilter } from '../../server/lib/helpers/range_filter';


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

-      context.__LEGACY.server
-    );
-    await internalSavedObjectsClient.create(
+    await savedObjectsClient.create(


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

@@ -12,7 +12,7 @@ import {
  SetupTimeRange,
  SetupUIFilters
 } from '../../../../helpers/setup_request';
-import { fetchAndTransformGcMetrics } from './fetchAndTransformGcMetrics';
+import { fetchAndTransformGcMetrics } from './fetch_and_transform_gc_metrics';


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

+export async function getInternalSavedObjectsClient(core: CoreSetup) {
+  return core.getStartServices().then(async ([coreStart]) => {
+    return coreStart.savedObjects.createInternalRepository();
+  });


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 7074e7b
💚 Build #27414 succeeded d6b6314
💔 Build #26705 failed 3ec54f2
💔 Build #26433 failed 6698349
💔 Build #26412 failed 0b888d4

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 review from a team as code owners 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

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 Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] NP Migration - Moves plugin server files out of legacy #57532

[APM] NP Migration - Moves plugin server files out of legacy #57532

ogupte commented Feb 13, 2020 •

edited

Loading

ogupte Feb 13, 2020

ogupte Feb 13, 2020

ogupte Feb 13, 2020

ogupte Feb 13, 2020

ogupte Feb 13, 2020

ogupte Feb 13, 2020

ogupte Feb 13, 2020 •

edited

Loading

ogupte Feb 13, 2020

sorenlouv Feb 13, 2020

ogupte Feb 14, 2020

sorenlouv Feb 14, 2020

watson Feb 18, 2020

watson Feb 18, 2020

sorenlouv Feb 18, 2020

ogupte Feb 13, 2020

sorenlouv Feb 14, 2020

sorenlouv Feb 14, 2020 •

edited

Loading

sorenlouv Feb 14, 2020

sorenlouv Feb 14, 2020

sorenlouv Feb 14, 2020

kindsun commented Feb 18, 2020

Kerry350 left a comment

kibanamachine commented Feb 19, 2020

ogupte commented Feb 19, 2020

[APM] NP Migration - Moves plugin server files out of legacy #57532

[APM] NP Migration - Moves plugin server files out of legacy #57532

Conversation

ogupte commented Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogupte Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kindsun commented Feb 18, 2020

Kerry350 left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 19, 2020

💚 Build Succeeded

History

ogupte commented Feb 19, 2020

ogupte commented Feb 13, 2020 •

edited

Loading

ogupte Feb 13, 2020 •

edited

Loading

sorenlouv Feb 14, 2020 •

edited

Loading