[APM] Migrate server routes to NP #49455

dgieselaar · 2019-10-28T08:26:31Z

Closes #49238.
Closes #51156

Changes:

Create new platform plugin for APM & APM OSS
Use APM OSS configuration in APM plugin, and merge/flatten config values
Move createApmApi and createApmAgentConfiguration to NP. Not sure how to move makeApmUsageSelector as it depends on server.usage.
Signature for routes changed from ( request:hapi.Request, params:Params, h:hapi.ResponseToolkit ) to ( kibanaContext:{ context: APMRequestHandlerContext, request: Kibana.Request } ).
APMRequestHandlerContext is Kibana.RequestHandlerContext plus the APM configuration and the decoded request parameters.
Range & UI filters that are used in setup_request now have type safety, ie if a query for an endpoint uses those filters, type check will fail if any validation is missing for those query parameters
Removed agentConfigurationIndex from configurable UI indices. Should be created and queried with internalUser, so making this configurable could lead to security issues.

TODOs/questions (cc @rudolf):

The apm/apm_oss legacy configurations are used by infra and the tutorial. In this PR, we have two schemas, one for NP with @kbn/config-schema, and one for LP with Joi. Is there a way we can prevent this duplication (and still expose legacy configuration values)? Copying and pasting stuff for now.
We are using io-ts for validation route input. NP requires a schema from @kbn/config-schema. Right now we're using a noop schema as a workaround, but it's not pretty. Any thoughts on how this could be better? Platform will enable support for io-ts in the near future, see Decouple request validation from @kbn/config-schema #50179
~~What is the replacement for server.usage?~~ Blocked by collectorSets in the New Platform #46924. Left in legacy plugin for now.

elasticmachine · 2019-10-28T09:11:54Z

💔 Build Failed

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

elasticmachine · 2019-11-01T14:12:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1945cd62fedb8e459cbbed5c738a6d813b1eef36

rudolf · 2019-11-01T20:00:11Z

Legacy vs NP config

There's two options, either you keep the duplicated legacy schema around until all dependencies have moved away from Legacy. Or you migrate the dependencies to pull these config values from the NP plugin for them. For instance I saw infra uses legacy config to get the apmIndex. The code below doesn't work cause there's no server in this context, but I hope it illustrates how you would wire this up.

If there's not a lot of places depending on this legacy config, it's probably best to go all out and only keep the NP config, changing all legacy plugins to consume this NP plugins' configuration.

diff --git a/x-pack/legacy/plugins/infra/server/routes/metadata/lib/has_apm_data.ts b/x-pack/legacy/plugins/infra/server/routes/metadata/lib/has_apm_data.ts
index 3193cf8397..7b91e67ea2 100644
--- a/x-pack/legacy/plugins/infra/server/routes/metadata/lib/has_apm_data.ts
+++ b/x-pack/legacy/plugins/infra/server/routes/metadata/lib/has_apm_data.ts
@@ -19,7 +19,11 @@ export const hasAPMData = async (
   nodeType: 'host' | 'pod' | 'container'
 ) => {
   const config = framework.config(req);
-  const apmIndex = config.get('apm_oss.transactionIndices') || 'apm-*';
+  // You'll probably want to extract apmConfig in infra's init.ts and then
+  // inject it into this function (through framework?)
+  const apmConfig = await server.newPlatform.setup.plugins.apm_oss.config$.toPromise();
+  const apmIndex = apmConfig.transactionIndices || 'apm-*';
+
   // There is a bug in APM ECS data where host.name is not set.
   // This will fixed with: https://github.com/elastic/apm-server/issues/2502
   const nodeFieldName =

Server.usage

Will be replaced by the metrics plugin in NP #46924 the stack services team is responsible for this, I think they're targeting 7.6

x-pack/legacy/plugins/apm/server/routes/create_api/index.ts

elasticmachine · 2019-11-12T21:24:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 7e53af71b70e2c13129a98aa86a26bbd6f80dab2

elasticmachine · 2019-11-13T15:49:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 112cf000a11bd7434ce62e4e73d3632ae7b6c397

mistic · 2019-11-13T19:39:44Z

@elasticmachine merge upstream

elasticmachine · 2019-11-13T19:39:46Z

merge conflict between base and head

mistic · 2019-11-13T21:33:34Z

@elasticmachine merge upstream

elasticmachine · 2019-11-13T22:31:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 3820c39754fe2c2f82a00582d894898e7e5fea5e

mistic · 2019-11-14T03:00:32Z

@elasticmachine merge upstream

elasticmachine · 2019-11-14T04:23:19Z

💔 Build Failed

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

dgieselaar · 2019-11-14T14:15:54Z

I moved this out of draft prematurely. I still have to figure out how to create an internal saved objects client.

elasticmachine · 2019-11-14T15:19:27Z

💔 Build Failed

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

elasticmachine · 2019-11-14T16:39:15Z

💔 Build Failed

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

elasticmachine · 2019-11-20T12:37:26Z

💚 Build Succeeded

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

x-pack/legacy/plugins/apm/server/lib/helpers/setup_request.ts

sorenlouv · 2019-11-20T14:46:18Z

x-pack/legacy/plugins/apm/server/lib/helpers/setup_request.ts

+  uiFiltersES: ESFilter[];
+};
+
+export type SetupWithAllFilters = SetupWithTimeRange & SetupWithUIFilters;


I'm not sure this'll work longer term. What happens when we add a new property that not every route needs? Then it's not "all" anymore.

Perhaps it's better to be explicit and type it in place as: Setup & TimeRange & UIFilters.

Not for this PR but: Setup itself is already pretty opaque. It contains a bunch of different stuff - perhaps it would make sense to split that up into smaller parts too?

++ on separating Setup{WithTimeRange,WithUIFilters} and using TS unions. Composable & explicit. Fixed.

I'd think it'd be good to think (at some point) about APMRequestContext and SetupRequest, how they overlap, how they are different, and possibly if we need a APMPluginContract or something similar as well. Maybe after NP migration is completed?

sorenlouv · 2019-11-20T14:55:46Z

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

+        }
+      }
+    }
+  } as unknown) as APMRequestHandlerContext;


What do you think about creating the saved objects client on Setup like we do with client and internalClient. then we could have savedObjectsClient and internalSavedObjectsClient. This way we wouldn't have to pass context around anymore.

context is still needed for config and some other stuff, or am I misunderstanding? the internal saved objects client is a WIP, maybe better to wait? #48882

config is already on Setup. I'm thinking that it would be nice if downstream handlers don't have to know about Context or Request - at least if they are just doing normal stuff like reading config, calling (internal) savedobjects or (internal) esClient.

Right, I see what you mean. Ok if I create an issue for this that we can pick up after the migration to NP is complete (including the usage collector and the internal saved objects client)?

Went ahead and created the issue: #51257

x-pack/legacy/plugins/apm/server/lib/index_pattern/get_dynamic_index_pattern.ts

sorenlouv · 2019-11-20T16:21:03Z

x-pack/legacy/plugins/apm/server/lib/services/get_services/index.ts

+export async function getServices(setup: SetupWithAllFilters) {
+  const itemsPromise = getServicesItems(setup);
+  const hasLegacyDataPromise = getLegacyDataStatus(setup);
+  const items = await itemsPromise;


Any reason for not await'ing immediately? This ends up being await'ed in serial endyway.

getLegacyDataStatus (2) is not blocked by getServicesItems (1), only hasHistoricalAgentData (3) is. So this starts 1 and 2 immediately, and if 1 finishes, 3 starts. Then the function returns when all promises are resolved. At least, that's what it should do. Do you see something else happening?

I could also rewrite with promises if it makes more sense:

const [{ items, hasHistoricalData }, hasLegacyData] = await Promise.all([ getServicesItems(setup).then(async servicesItems => { const noDataInCurrentTimeRange = isEmpty(servicesItems); return { items: servicesItems, hasHistoricalData: noDataInCurrentTimeRange ? await hasHistoricalAgentData(setup) : true }; }), getLegacyDataStatus(setup) ]);

WDYT?

Maybe I'm missing something but isn't this equivalent to using Promise.all for the first two promises?

export async function getServices(setup: SetupWithAllFilters) { const [items, hasLegacyData] = await Promise.all([ getServicesItems(setup), getLegacyDataStatus(setup) ]); const noDataInCurrentTimeRange = isEmpty(items); const hasHistoricalData = noDataInCurrentTimeRange ? await hasHistoricalAgentData(setup) : true; return { items, hasHistoricalData, hasLegacyData }; }

At least this approach is more clear to me.

getServicesItems could finish before getLegacyDataStatus and then the latter would unnecessary block hasHistoricalAgentData, right? (hasHistoricalAgentData needs getServicesItems, but not hasLegacyData)

Maybe it's not worth it though? I do think the version that we have today is more readable for sure.

getServicesItems could finish before getLegacyDataStatus and then the latter would unnecessary block hasHistoricalAgentData, right?

Did you time this? I'd expect in most cases getLegacyDataStatus to be faster than getServicesItems - ofc this depends on both the selected time range and the data volumes.
Regardless, I think the difference is negligible and I'd optimize for the most readable version.

I didn't time it, I figured it would be a win anyway but agreed, let's keep it simple 👍

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

sorenlouv · 2019-11-20T16:27:56Z

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

+  schema: schema.object({
+    servicemapEnabled: schema.boolean({ defaultValue: false }),
+    bucketTargetCount: schema.number({ defaultValue: 15 }),
+    minimumBucketSize: schema.number({ defaultValue: 15 }),


minimumBucketSize and bucketTargetCount are carry-over from the opbeat days (yeah!).
If you think it makes sense in this PR it would be great if you can just make them constants (like apmAgentConfigurationIndex)

Moved these to plain old constants in the server/lib/transactions/constants. They're not used outside of transactions AFAICT.

elasticmachine · 2019-11-20T21:33:08Z

💚 Build Succeeded

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

sorenlouv

looks great only a few nitpicks around how we can handle context, Setup etc.

Closes elastic#49238. Pass legacy API to NP plugin in order to use internal SO client Fix issues with agent configuration APIs Update tsconfig template for TS optimization Fix typo

elasticmachine · 2019-11-21T09:42:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5bf33fd

rudolf

Left some small suggestions, but all in all this looks great!

rudolf · 2019-11-21T10:25:51Z

x-pack/legacy/plugins/apm/server/routes/typings.ts

+  config: APMConfig;
+  logger: Logger;
+  __LEGACY: {
+    server: Server;


nit: You're using a really broad type here which makes it hard to see which dependencies you're using. I would suggest just typing the dependencies you're using like:

__LEGACY: { server: { savedObjects: { getScopedSavedObjectsClient: Server['savedObjects']['getScopedSavedObjectsClient'] } // similar for usage collector } }

Good one, I've narrowed the type to the things that we are using at the moment.

rudolf · 2019-11-21T10:27:48Z

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

+import { once } from 'lodash';
+import { Plugin as APMOSSPlugin } from '../../../../src/plugins/apm_oss/server';
+import { createApmAgentConfigurationIndex } from '../../../legacy/plugins/apm/server/lib/settings/agent_configuration/create_agent_config_index';
+import { createApmApi } from '../../../legacy/plugins/apm/server/routes/create_apm_api';


nit: it's a bit confusing when you have routes in a legacy plugin, being run from a NP plugin, but injecting dependencies from a legacy plugin, just kinda hard to wrap your head around. Do you still need these files in legacy or could you move create_apm_api and all the routes into the NP plugin?

I think this makes more sense to do when the migration is completed. Otherwise legacy will import lots of stuff from np and the other way around. I've added it to the list in #32894

elasticmachine · 2019-11-21T14:44:38Z

💚 Build Succeeded

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

* [APM] Migrate server routes to NP Closes #49238. Pass legacy API to NP plugin in order to use internal SO client Fix issues with agent configuration APIs Update tsconfig template for TS optimization Fix typo * Review feedback * Fix type issues after browser duration changes * Revert changes in getServices due to readability concerns * Review feedback

dgieselaar added release_note:enhancement v7.6.0 blocked labels Oct 28, 2019

dgieselaar force-pushed the migrate-create-router branch 3 times, most recently from 85ac256 to 1945cd6 Compare November 1, 2019 13:29

dgieselaar mentioned this pull request Nov 1, 2019

[APM] Don't read APM index names directly from config #49620

Closed

2 tasks

rudolf reviewed Nov 1, 2019

View reviewed changes

x-pack/legacy/plugins/apm/server/routes/create_api/index.ts Show resolved Hide resolved

dgieselaar force-pushed the migrate-create-router branch from 1945cd6 to 7e53af7 Compare November 12, 2019 20:29

dgieselaar force-pushed the migrate-create-router branch from 7e53af7 to 112cf00 Compare November 13, 2019 14:42

dgieselaar force-pushed the migrate-create-router branch from 112cf00 to 0591d08 Compare November 13, 2019 21:31

dgieselaar force-pushed the migrate-create-router branch 2 times, most recently from b24e7b2 to 6b597e1 Compare November 14, 2019 13:45

dgieselaar marked this pull request as ready for review November 14, 2019 13:49

dgieselaar requested a review from a team as a code owner November 14, 2019 13:49

dgieselaar force-pushed the migrate-create-router branch from 6b597e1 to e5b6590 Compare November 14, 2019 13:56

dgieselaar force-pushed the migrate-create-router branch 2 times, most recently from 25e8004 to c0d153a Compare November 20, 2019 11:20

dgieselaar mentioned this pull request Nov 20, 2019

[APM] Make agent configuration index non-configurable #51156

Closed

sorenlouv reviewed Nov 20, 2019

View reviewed changes

x-pack/legacy/plugins/apm/server/lib/helpers/setup_request.ts Show resolved Hide resolved

sorenlouv reviewed Nov 20, 2019

View reviewed changes

x-pack/legacy/plugins/apm/server/lib/index_pattern/get_dynamic_index_pattern.ts Show resolved Hide resolved

dgieselaar mentioned this pull request Nov 20, 2019

Kibana NP migration: homeApi -> registerTutorials #50224

Closed

sorenlouv reviewed Nov 20, 2019

View reviewed changes

x-pack/legacy/plugins/apm/server/lib/settings/apm_indices/get_apm_indices.ts Show resolved Hide resolved

sorenlouv reviewed Nov 20, 2019

View reviewed changes

dgieselaar force-pushed the migrate-create-router branch from c0d153a to c322965 Compare November 20, 2019 20:01

sorenlouv approved these changes Nov 20, 2019

View reviewed changes

dgieselaar mentioned this pull request Nov 21, 2019

[APM] Plugin & request context and query setup #51257

Closed

dgieselaar added 4 commits November 21, 2019 09:24

[APM] Migrate server routes to NP

9b368b4

Closes elastic#49238. Pass legacy API to NP plugin in order to use internal SO client Fix issues with agent configuration APIs Update tsconfig template for TS optimization Fix typo

Review feedback

354fa45

Fix type issues after browser duration changes

b433369

Revert changes in getServices due to readability concerns

5bf33fd

dgieselaar force-pushed the migrate-create-router branch from fdf6efa to 5bf33fd Compare November 21, 2019 08:24

rudolf approved these changes Nov 21, 2019

View reviewed changes

Review feedback

c024a48

dgieselaar merged commit 06bee60 into elastic:master Nov 21, 2019

dgieselaar deleted the migrate-create-router branch November 21, 2019 14:46

dgieselaar mentioned this pull request Nov 21, 2019

[7.x] [APM] Migrate server routes to NP (#49455) #51289

Merged

dgieselaar mentioned this pull request Nov 26, 2019

[APM] Address fallout from NP server migration #51725

Merged

[APM] Migrate server routes to NP #49455

[APM] Migrate server routes to NP #49455

Conversation

dgieselaar commented Oct 28, 2019 • edited by sorenlouv Loading

elasticmachine commented Oct 28, 2019

💔 Build Failed

elasticmachine commented Nov 1, 2019

💔 Build Failed

rudolf commented Nov 1, 2019

Legacy vs NP config

Server.usage

elasticmachine commented Nov 12, 2019

💔 Build Failed

elasticmachine commented Nov 13, 2019

💔 Build Failed

mistic commented Nov 13, 2019

elasticmachine commented Nov 13, 2019

mistic commented Nov 13, 2019

elasticmachine commented Nov 13, 2019

💔 Build Failed

mistic commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

💔 Build Failed

dgieselaar commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

💔 Build Failed

elasticmachine commented Nov 14, 2019

💔 Build Failed

elasticmachine commented Nov 20, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

dgieselaar Nov 20, 2019 • 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

dgieselaar Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

dgieselaar Nov 21, 2019 • 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

elasticmachine commented Nov 20, 2019

💚 Build Succeeded

sorenlouv left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 21, 2019

💔 Build Failed

rudolf left a 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

elasticmachine commented Nov 21, 2019

💚 Build Succeeded

dgieselaar commented Oct 28, 2019 •

edited by sorenlouv

Loading

dgieselaar Nov 20, 2019 •

edited

Loading

dgieselaar Nov 20, 2019 •

edited

Loading

sorenlouv Nov 20, 2019 •

edited

Loading

dgieselaar Nov 21, 2019 •

edited

Loading