[APM] Service Map data background task and API endpoint #50844

ogupte · 2019-11-16T01:19:49Z

Addresses #48996.

Adds data later for the service maps feature in APM:

creates new index apm-service-connections
schedules periodic task that stores service connection documents in the apm-service-connections index
implements endpoint /api/apm/service-map that query/filters service connections to render the service map

In order to test this out without agents setting destination.address, you can install an ingest pipeline that approximates agent support:

git clone https://github.com/ogupte/apm-ui-seed.git
cd apm-ui-seed
yarn
bin/apm-ui-service-map-ingest --host='http://localhost:9200' --auth='elastic:password' install

when you're done testing, you can uninstall the dev pipeline with:

bin/apm-ui-service-map-ingest --host='http://localhost:9200' --auth='elastic:password' uninstall

elasticmachine · 2019-11-16T02:43:41Z

💔 Build Failed

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

elasticmachine · 2019-11-22T11:50:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 16b45f0dee0cf23d8d0628ea68270bc9b32aae23

elasticmachine · 2019-11-26T09:25:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 2ea43584dfeaf6e8d265b0a484f34ab6b0c6381c

dgieselaar · 2019-11-28T20:49:22Z

x-pack/legacy/plugins/apm/server/lib/service_map/initialize_service_maps.ts

+      if (fetchedTasks.docs.length) {
+        await taskManager.remove('servicemap-processor');
+      }
+      await taskManager.schedule({


I'm not sure if the point of this is to prevent this task being scheduled more than once, but if that is the case, then you can now use ensureScheduled: #50232

sorenlouv · 2019-12-03T16:23:49Z

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

+
+      if (config.get('xpack.apm.serviceMapEnabled')) {
+        initializeServiceMaps(server).catch(error => {
+          throw error;


This will result in an unhandled promise exception

sorenlouv · 2019-12-03T16:25:07Z

x-pack/legacy/plugins/apm/server/lib/service_map/get_service_map.ts

+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+import { idx } from '@kbn/elastic-idx';


idx doing a comeback??? :D

sorenlouv · 2019-12-03T16:26:32Z

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

      if (start && end) {
        return callApmApi({
-          pathname: '/api/apm/service-map',
-          params: { query: { start, end } }
+          pathname: '/api/apm/service-map/all',


What if a service is called "all"?

sorenlouv · 2019-12-03T16:40:09Z

x-pack/legacy/plugins/apm/server/lib/service_map/get_service_map.ts

+    });
+  }
+
+  // @ts-ignore


Is this still needed?

sorenlouv · 2019-12-03T21:04:14Z

src/legacy/core_plugins/apm_oss/index.js

@@ -38,18 +38,24 @@ export default function apmOss(kibana) {
        spanIndices: Joi.string().default('apm-*'),
        metricsIndices: Joi.string().default('apm-*'),
        onboardingIndices: Joi.string().default('apm-*'),
+        apmAgentConfigurationIndex: Joi.string().default('.apm-agent-configuration')


Was it intentional to re-add this?

sorenlouv · 2019-12-03T21:12:22Z

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

+              start,
+              end,
+              environment,
+              uiFilters: JSON.stringify(uiFiltersOmitEnv)


We normally don't send environment like a separate query params but just keep it in uiFilters. Can't we do the same here?

sorenlouv · 2019-12-03T21:13:14Z

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

 export const serviceMapRoute = createRoute(() => ({
-  path: '/api/apm/service-map',
+  path: '/api/apm/service-map/{serviceName}',


As we talked about /api/apm/service-map/{serviceName} and /api/apm/service-map/all can be combined to a single route if serviceName is converted to an optional query param

sorenlouv · 2019-12-03T21:13:47Z

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

-      return getServiceMap();
+  handler: async ({ context, request }) => {
+    if (!context.config['xpack.apm.serviceMapEnabled']) {
+      return new Boom('Not found', { statusCode: 404 });


serviceMapEnabled should be removed

... or, are there any reasons to let users disable it? We don't allow this for any other features.

yeah we can safely remove this config.

after talking with folks, the consensus was to keep the config until FF

what's the reason to wait until FF if we decided to do this?

sorenlouv · 2019-12-03T21:15:45Z

x-pack/legacy/plugins/apm/server/lib/service_map/get_service_map.ts

+        };
+      }>;
+    };
+  }> = idx(aggregations, _ => _.conns.buckets) || [];


@dgieselaar is this something our magic agg types should be able to infer instead of them being spelled out?

Looks like this is no longer an issue, correct?

sorenlouv · 2019-12-03T21:17:01Z

x-pack/legacy/plugins/apm/server/lib/service_map/initialize_service_maps.ts

+      'Unable to set up required scripts for APM Service Maps:'
+    );
+    server.log('error', error);
+  });


Is it the intention to continue initialization if setupRequiredScripts fails?

that was the idea, the result will be some error messages in kibana logs & service maps UI not displaying any nodes

How's that useful? What about just terminating initialization as soon as something goes wrong?

Is this still being worked on?

this file has undergone a metamorphosis since then. It's not responsible for initializing the endpoint that a user hits to kick off the task in that user's scope. This endpoint had to be created here since it needs access to the task manager, so it wasn't able to live with the other APM APIs.

sorenlouv · 2019-12-03T21:20:09Z

x-pack/legacy/plugins/apm/server/lib/service_map/initialize_service_maps.ts

+                term: {
+                  'task.taskType': 'serviceMap'
+                }
+              }


Nit: what do you think about single-lining terms like this:

{ term: { _id: "task:servicemap-processor" } }, { term: { "task.taskType": "serviceMap" } }

sorenlouv · 2019-12-03T21:20:57Z

x-pack/legacy/plugins/apm/server/lib/service_map/run_service_map_task.ts

+
+function interestingTransactions(since?: string, afterKey?: any) {
+  if (!since) {
+    since = 'now-1h';


How was this value chosen? Should it be a constant?

Btw what about adding it to the function signature?

sorenlouv · 2019-12-03T21:26:31Z

x-pack/legacy/plugins/apm/server/lib/service_map/run_service_map_task.ts

+    query: {
+      bool: {
+        filter: [
+          { exists: { field: 'destination.address' } },


Use constants

sorenlouv · 2019-12-03T21:27:29Z

x-pack/legacy/plugins/apm/server/lib/service_map/run_service_map_task.ts

+          { exists: { field: 'destination.address' } },
+          { exists: { field: 'trace.id' } },
+          { exists: { field: 'span.duration.us' } },
+          { range: { '@timestamp': { gt: since } } }


What about using start and end like in the rest of the codebase?

Am I correct in understanding that end is currently ignored?

Ah, nvm. This is not the function called by the end-user but the task manager.

sorenlouv · 2019-12-03T21:30:57Z

x-pack/legacy/plugins/apm/server/lib/service_map/run_service_map_task.ts

+      body
+    });
+  }
+}


As we talked about, I think this file would benefit from being refactored into smaller functions, having better types, and being less mutable (if possible).

sorenlouv · 2019-12-03T21:32:46Z

x-pack/legacy/plugins/apm/server/lib/service_map/setup_required_scripts.ts

+
+            return new ArrayList(conns)
+          }
+          return [];


Do we have tests for this? I think that's quiet important since we have no static safety - not even syntax highlighting - to notify us of any issues.

Btw ran it through a formatter to fix some minor indentation and spacing issues:`

void extractChildren(def caller, def spans, def upstream, def conns, def count) { // TODO: simplify this if (spans.containsKey(caller.id)) { for (s in spans[caller.id]) { if (caller.span_type == 'external') { upstream.add(caller.service_name + "/" + caller.environment); def conn = new HashMap(); conn.caller = caller; conn.callee = s; conn.upstream = new ArrayList(upstream); conns.add(conn); extractChildren(s, spans, upstream, conns, count); upstream.remove(upstream.size() - 1); } else { extractChildren(s, spans, upstream, conns, count); } } } else { // no connection found def conn = new HashMap(); conn.caller = caller; conn.upstream = new ArrayList(upstream); conn.upstream.add(caller.service_name + "/" + caller.environment); conns.add(conn); } } def conns = new HashSet(); def spans = new HashMap(); // merge results from shards for (state in states) { for (s in state.entrySet()) { def v = s.getValue(); def k = s.getKey(); if (!spans.containsKey(k)) { spans[k] = v; } else { for (p in v) { spans[k].add(p); } } } } if (spans.containsKey(null) && spans[null].size() > 0) { def node = spans[null][0]; def upstream = new ArrayList(); extractChildren(node, spans, upstream, conns, 0); return new ArrayList(conns) } return [];

sorenlouv · 2019-12-03T21:43:45Z

x-pack/legacy/plugins/apm/server/lib/service_map/setup_required_scripts.ts

+        source: `
+          void extractChildren(def caller, def spans, def upstream, def conns, def count) {
+            // TODO: simplify this
+            if (spans.containsKey(caller.id)) {


Is it still the plan to simplify this?

sorenlouv · 2019-12-03T21:45:06Z

x-pack/legacy/plugins/apm/server/lib/service_map/setup_required_scripts.ts

+            // TODO: simplify this
+            if (spans.containsKey(caller.id)) {
+              for(s in spans[caller.id]) {
+                  if (caller.span_type=='external') {


I see external being used a couple of places. Should it be a constant like we have for transaction types?

sorenlouv · 2019-12-03T21:50:09Z

x-pack/legacy/plugins/apm/server/lib/service_map/setup_required_scripts.ts

+            if: "ctx.span != null && ctx.span.type == 'ext'",
+            field: 'span.type',
+            value: 'external'
+          }


Looks like we rewrite ext to external. Why is that?

At the time, the value was not consistent, so this normalized everything to external.

see elastic/apm-agent-nodejs#1225 (comment)

should also note that this ingest pipeline will not be merged.

sorenlouv · 2019-12-03T21:51:37Z

x-pack/legacy/plugins/apm/server/lib/service_map/setup_required_scripts.ts

+    // get current apm ingest pipeline
+    apmIngestPipeline = await getIngestPipelineApm(callCluster);
+  } catch (error) {
+    if (error.statusCode !== 404) {


Maybe add a comment about why we are swallowing 404 errors

nvm. comment below is sufficient 👍

sorenlouv · 2019-12-03T21:57:11Z

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

@@ -33,6 +36,9 @@ export function mergeConfigs(apmOssConfig: APMOSSConfig, apmConfig: APMXPackConf
    'xpack.apm.ui.maxTraceItems': apmConfig['ui.maxTraceItems'],
    'xpack.apm.ui.transactionGroupBucketSize': apmConfig['ui.transactionGroupBucketSize'],
    'xpack.apm.autocreateApmIndexPattern': apmConfig.autocreateApmIndexPattern,
+    'xpack.apm.serviceMapIndexPattern': apmConfig.serviceMapIndexPattern,


serviceMapIndexPattern is misleading. It's not an index pattern (in the Kibana sense). This should follow the names of the other keys: serviceMapIndices.
Even though this is an xpack feature I'm contemplating whether it'll make more sense to keep this in apm_oss where all other indices are configured.

I don't think it belongs in the apm_oss set of configs, since it's a premium feature. it will not be accessed if the use doesn't pass a license check.

Yeah, I know, but imo it feels weird as an xpack user that you have to configure your indices differently. Let's get a second opinion on this.

@bmorelli25 i'm interested in your take on this. Currently all of the apm indicies are namespaced under apm.oss. Examples: apm_oss.errorIndices, apm_oss.transactionIndices and apm_oss.spanIndices. Now we are also going to add a setting for Service Maps indices.

From a docs perspective what do you think makes more sense to:

apm_oss.serviceMapIndices (consistency with other index configuration option names)

xpack.apm.serviceMapIndices (Service Maps are only available in basic so is not used for anything in oss)

(Service Maps are only available in basic...)

should only be available in platinum

My vote is for xpack.apm.serviceMapIndices.

it feels weird as an xpack user that you have to configure your indices differently.

I agree, it does feel a little weird, but I think it's less weird than namespacing a platinum feature under the oss prefix.

Makes sense. Then let's stick to xpack.apm.serviceMapIndices.

sorenlouv · 2019-12-18T10:49:47Z

x-pack/legacy/plugins/apm/server/lib/service_map/get_service_map.ts

+
+    params.body.query.bool.filter.push({
+      wildcard: {
+        [CONNECTION_UPSTREAM_LIST]: `${upstreamServiceName}/${upstreamEnvironment}`


Maybe this discussion has come up before but how much time have we spent looking into having service.name and service.environment in separate fields?
If the problem is due to querying (because it's an array of objects) could we used nested fields instead?

That would make querying faster and imo simpler (although nested fields does introduce a slightly different syntax)

@dgieselaar any opinion on this?

perhaps do both? service.name, service.environment, and service.identifier (or something more appropriately named). I'm not sure if we should use nested, aggregations become more complicated and it doesn't work with index sorting, which we will probably not use in practice but I think the use case for nested needs to be really compelling as there are some drawbacks.

My main concern with the current approach is that it is not longer ECS compliant so we can't apply ui filters like normally. Additionally wildcard can be terribly slow.

I'm fine with having both for possible future use cases. Although currently I don't think we want to query on CONNECTION_UPSTREAM_LIST

I'll have a proper look now, might be missing some context

sorenlouv · 2019-12-18T21:34:42Z

x-pack/legacy/plugins/apm/server/lib/service_map/run_service_map_task.ts

+      )
+    );
+    return traceConnections.flatMap(
+      mapTraceToBulkServiceConnection(apmIndices, servicesInTrace)


nit: Making mapTraceToBulkServiceConnection a higher-order function seems slightly unnecessary. Perhaps just:

return traceConnections.flatMap(traceConnection => mapTraceToBulkServiceConnection(apmIndices, servicesInTrace, traceConnection)

sorenlouv · 2019-12-18T21:38:37Z

x-pack/legacy/plugins/apm/server/lib/service_map/get_next_transaction_samples.ts

+  esClient: ESClient;
+}) {
+  const params = {
+    index: targetApmIndices,


It's not immediately possible to see which indices are being queried. Everywhere else apmIndices is passed in. Can we do the same here?

Suggested change

index: targetApmIndices,

index: [

apmIndices['apm_oss.transactionIndices'],

apmIndices['apm_oss.spanIndices']

]

sorenlouv · 2019-12-18T21:39:38Z

x-pack/legacy/plugins/apm/server/lib/service_map/get_service_connections.ts

+  esClient: ESClient;
+}) {
+  const params = {
+    index: targetApmIndices,


Same as above

sorenlouv · 2019-12-18T21:46:59Z

x-pack/legacy/plugins/apm/server/lib/service_map/get_next_transaction_samples.ts

+                }
+              },
+              aggs: {
+                tracesample: {


Suggested change

tracesample: {

trace_sample: {

sorenlouv · 2019-12-18T22:02:30Z

x-pack/legacy/plugins/apm/server/lib/service_map/service-connection-es-scripts.ts

+      'span.subtype',
+      'service.name',
+      'service.environment',
+      'destination.address'


What do you think about using constants here?

I considered it but felt it was best to avoid interpolation of the script source. I didn't want it to look like i was generating dynamic scripts from our code.

sorenlouv · 2019-12-18T22:06:26Z

x-pack/legacy/plugins/apm/server/lib/service_map/service-connection-es-scripts.ts

+  source: `return state.mappedDocs`
+};
+
+export const reduceServiceConnsScript = {


Can you add a comment for each of these scripts about what they are doing?

- creates required scripts in elasticsearch - adds ingest pipeline that sets destination.address from span names - schedules periodic task that create connection documents - implements endpoints that query/filters connections to render the service map

- replace this.kbnServer.afterPluginsInit and task object fetch with taskManager.ensureScheduled - simplify routes: move serviceName to query param and consolidate endpoints - clean up code in get_service_map.ts, constants, typescript fixes - add typescript support for composite aggregations

- broke up the task into more files for better organization and testing

… them in ES.

….address from span names

- conform to ESC fields - performance improvments to get service connections - add checks for empty bulk requests

default security settings. The kibana user is responsible for creating and index to the `apm-service-connections` data index, while the apm user is resposible for kicking off the scheduled task and reading from `apm-*` indices.

kibanamachine · 2019-12-21T11:02:40Z

💔 Build Failed

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

History

💔 Build #15584 failed 02870016934cfb3ef73f2fcbb63a63c20af5005f
💔 Build #14704 failed 590ceba4a828e42bf68fce7ff53062fba5bf8dca
💔 Build #14475 failed a5edd893e54fc7de40768b42e604e850a602fb77
💔 Build #14454 failed 06b07ee2a7baf5d62fb7813a154a45adbb7f3793
💔 Build #14417 failed ec845675f89142b7e7b4a1fb5b1e90a27b139916

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

ogupte · 2020-01-11T02:10:16Z

closing this in favor of the runtime implementation #54027

ogupte added the v7.6.0 label Nov 16, 2019

ogupte self-assigned this Nov 16, 2019

ogupte force-pushed the apm-48996-service-map-data branch from a07a4f8 to 16b45f0 Compare November 22, 2019 10:25

ogupte force-pushed the apm-48996-service-map-data branch 2 times, most recently from 16b45f0 to 2ea4358 Compare November 26, 2019 08:12

dgieselaar reviewed Nov 28, 2019

View reviewed changes

sorenlouv reviewed Dec 3, 2019

View reviewed changes

x-pack/legacy/plugins/apm/server/lib/service_map/get_service_map.ts Outdated

});

}

// @ts-ignore

Copy link

Member

sorenlouv Dec 3, 2019

Choose a reason for hiding this comment

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

Is this still needed?

sorenlouv reviewed Dec 3, 2019

View reviewed changes

ogupte force-pushed the apm-48996-service-map-data branch from 590ceba to 0287001 Compare December 14, 2019 13:17

ogupte marked this pull request as ready for review December 16, 2019 17:24

ogupte requested a review from a team as a code owner December 16, 2019 17:24

sorenlouv reviewed Dec 18, 2019

View reviewed changes

ogupte added 15 commits December 21, 2019 01:38

- rewrite the task run logic to be more readable and less procedural

658f048

- broke up the task into more files for better organization and testing

fix bug in query and timestamp

dc930e8

Include scripted_metric scripts with the query rather than persisting…

0939da8

… them in ES.

replace field strings with shared constants

e4860c5

Addressed feedback and fixed timestamp snapshot

577297d

creates apm-service-connections index and adds it to getApmIndices

0a3e0ae

generalize getEsClient to leverage it in run_service_map_task

14a231f

remove development helper ingest pipeline which extracted destination…

4e2ce54

….address from span names

fix tests to snapshot ISO 8601 timestamps

945d8b0

- code improvements to es scripts

2121bcd

- conform to ESC fields - performance improvments to get service connections - add checks for empty bulk requests

fix import path failures

5e62db8

fixes typescript and task api errors

d7734ff

ogupte force-pushed the apm-48996-service-map-data branch from 0287001 to f0aec81 Compare December 21, 2019 09:48

elastic deleted a comment from elasticmachine Dec 21, 2019

ogupte closed this Jan 11, 2020

-    index: targetApmIndices,
+    index: [
+      apmIndices['apm_oss.transactionIndices'],
+      apmIndices['apm_oss.spanIndices']
+    ]

[APM] Service Map data background task and API endpoint #50844

[APM] Service Map data background task and API endpoint #50844

Conversation

ogupte commented Nov 16, 2019 • edited Loading

elasticmachine commented Nov 16, 2019

💔 Build Failed

elasticmachine commented Nov 22, 2019

💔 Build Failed

elasticmachine commented Nov 26, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Dec 3, 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

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 Dec 4, 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

sorenlouv Dec 3, 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

sorenlouv Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Dec 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogupte Dec 5, 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

dgieselaar Dec 18, 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

sorenlouv Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

sorenlouv Dec 18, 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

kibanamachine commented Dec 21, 2019

💔 Build Failed

History

ogupte commented Nov 16, 2019 •

edited

Loading

sorenlouv Dec 3, 2019 •

edited

Loading

sorenlouv Dec 4, 2019 •

edited

Loading

sorenlouv Dec 3, 2019 •

edited

Loading

sorenlouv Dec 3, 2019 •

edited

Loading

sorenlouv Dec 3, 2019 •

edited

Loading

sorenlouv Dec 4, 2019 •

edited

Loading

ogupte Dec 5, 2019 •

edited

Loading

dgieselaar Dec 18, 2019 •

edited

Loading

sorenlouv Dec 18, 2019 •

edited

Loading

sorenlouv Dec 18, 2019 •

edited

Loading