Service Map Data API at Runtime #54027

ogupte · 2020-01-06T17:15:39Z

Addresses #48996.

Implements the API for service map data which is managed at runtime in the client.

smith · 2020-01-09T19:04:16Z

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

@@ -62,17 +62,30 @@ export function Cytoscape({
  // Trigger a custom "data" event when data changes
  useEffect(() => {
    if (cy) {
+      cy.remove(cy.nodes());


Do we need to do the remove? As long as each node and edge have a unique id we should be able to add elements without removing. We might want to make this component use memo to only update if the list of ids changes.

smith · 2020-01-09T19:04:32Z

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

      cy.add(elements);
      cy.trigger('data');
    }
  }, [cy, elements]);

+  useEffect(() => {


This should be removed, since clicking on a node will bring up a popover.

smith · 2020-01-09T19:08:33Z

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


 interface ServiceMapProps {
  serviceName?: string;
 }

+type Element =


Not sure this is necessary with the built-in types in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/cytoscape/index.d.ts.

smith · 2020-01-09T19:23:05Z

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

@@ -37,37 +70,271 @@ ${theme.euiColorLightShade}`,
  margin: `-${theme.gutterTypes.gutterLarge}`
 };

+const MAX_REQUESTS = 15;
+
+function getConnectionNodeId(


Let's move a bunch of this to another file.

elasticmachine · 2020-01-11T02:08:57Z

Pinging @elastic/apm-ui (Team:apm)

dgieselaar

Some small suggestions, but almost there!

dgieselaar · 2020-01-09T21:16:43Z

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

@@ -71,7 +71,8 @@ export const apm: LegacyPluginInitializer = kibana => {
        autocreateApmIndexPattern: Joi.boolean().default(true),

        // service map
-        serviceMapEnabled: Joi.boolean().default(false)
+        serviceMapEnabled: Joi.boolean().default(false),
+        serviceMapInitialTimeRange: Joi.number().default(60 * 1000 * 15) // last 15 minutes


I'm okay with 15 minutes, but 1hr seemed fast enough for me.

dgieselaar · 2020-01-09T21:17:50Z

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

@@ -37,37 +70,271 @@ ${theme.euiColorLightShade}`,
  margin: `-${theme.gutterTypes.gutterLarge}`
 };

+const MAX_REQUESTS = 15;


Should this still be 15?

dgieselaar · 2020-01-09T21:19:11Z

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

+  if (renderedElements.current.length === 0) {
+    renderedElements.current = elements;
+  } else if (newData.length && !openToast.current) {
+    openToast.current = notifications.toasts.add({


btw, I'm not sure if we agreed on doing this. It was mostly to showcase how it looks. (FWIW, I think it's the best experience we can offer).

dgieselaar · 2020-01-09T21:21:14Z

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

+                /* if there is an outgoing span, create a new path */
+                if (event['destination.address'] != null && event['destination.address'] != '') {
+                  def outgoingLocation = new HashMap();
+                  outgoingLocation['destination.address'] = event['destination.address'];


We might want to add the span.type here as well, for the popovers.

span.type and span.subtype are already included in the external connection objects, are you saying they should also be part of the service connection objects as well?

I think this was an old comment that I didn't discard 😅 you can ignore it, it's there already indeed

dgieselaar · 2020-01-13T11:25:28Z

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

@@ -62,6 +62,7 @@ export function Cytoscape({
  // Trigger a custom "data" event when data changes
  useEffect(() => {
    if (cy) {
+      // cy.remove(cy.nodes());


is this not needed? cytoscape will dedupe automatically by id and id only? if so, we can remove this line completely right?

correct, i just forgot to remove this line

dgieselaar · 2020-01-13T11:40:51Z

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

+
+                    if (parent['destination.address'] != null
+                      && parent['destination.address'] != ""
+                      && parent['span.type'] == 'external'


we should add span.type == 'messaging' here, I think

so && parent['span.type'] == 'external' || parent['span.type'] == 'messaging'? is this for gRPC?

just messaging systems in general, not sure what kind of practical implementations there are

dgieselaar · 2020-01-13T11:41:16Z

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

+                }
+
+                /* if there is an outgoing span, create a new path */
+                if (event['destination.address'] != null && event['destination.address'] != '') {


we should only do this for span.type == 'messaging' | 'external' IIRC

dgieselaar · 2020-01-13T11:41:52Z

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

+
+                def lastLocation = basePath.size() > 0 ? basePath[basePath.size() - 1] : null;
+
+                def currentLocation = new HashMap(service);


I think we can just use def currentLocation = service; here

dgieselaar · 2020-01-13T11:44:15Z

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

+        if (serviceName) {
+          matches =
+            matches &&
+            'service.name' in node &&
+            node['service.name'] === serviceName;
+        }


This actually doesn't take the discovered services (destination.address => service.name) into account. Not sure whether that's an issue though, and we'll never get it 100% right, because an earlier or later request might have discovered a service that this specific request doesn't see.

dgieselaar · 2020-01-13T11:50:32Z

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

+    search
+  ]);
+
+  const forceUpdate = useCallback(() => _setUnusedState(value => !value), []);


This was kind of a hack, and we can probably make it a little better by just using setState properly. We can also do that a little later though.

Agree, would be nice to find a more elegant solution.

`xpack.apm.serviceMapInitialTimeRange` default to last 1 hour in milliseconds

fetching sample trace ids

…action during loading

…ace ids

- only show the update map button toast after last request loads

smith

dgieselaar

LGTM, let's get it in master :)

kibanamachine · 2020-01-13T21:16:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 02cae50

History

💚 Build #19601 succeeded 0bcf5f8891fe5596dec5be5beb08ef1fcaf63dc7
💚 Build #19491 succeeded 1de9a5084742975641076968e5fb3ad084506a4a
💚 Build #19246 succeeded b4d9480
💔 Build #19176 failed ebf6a39
💔 Build #19071 failed 8a987ea

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

* [APM] Runtime service maps * Make nodes interactive * Don't use smaller range query on initial request * Address feedback from Ron * Get all services separately * Get single service as well * Query both transactions/spans for initial request * Optimize 'top' query for service maps * Use agent.name from scripted metric * adds basic loading overlay * filter out service map node self reference edges from being rendered * Make service map initial load time range configurable with `xpack.apm.serviceMapInitialTimeRange` default to last 1 hour in milliseconds * ensure destination.address is not missing in the composite agg when fetching sample trace ids * wip: added incremental data fetch & progress bar * implement progressive loading design while blocking service map interaction during loading * adds filter that destination.address exists before fetching sample trace ids * reduce pairs of connections to 1 bi-directional connection with arrows on both ends of the edge * Optimize query; add update button * Allow user interaction after 5s, auto update in that time, otherwise show toast for user to update the map with button * Correctly reduce nodes/connections * - remove non-interactive state while loading - use cytoscape element definition types * - readability improvements to the ServiceMap component - only show the update map button toast after last request loads * addresses feedback for changes to the Cytoscape component * Add span.type/span.subtype do external nodes * PR feedback Co-authored-by: Dario Gieselaar <[email protected]>

* [APM] Runtime service maps * Make nodes interactive * Don't use smaller range query on initial request * Address feedback from Ron * Get all services separately * Get single service as well * Query both transactions/spans for initial request * Optimize 'top' query for service maps * Use agent.name from scripted metric * adds basic loading overlay * filter out service map node self reference edges from being rendered * Make service map initial load time range configurable with `xpack.apm.serviceMapInitialTimeRange` default to last 1 hour in milliseconds * ensure destination.address is not missing in the composite agg when fetching sample trace ids * wip: added incremental data fetch & progress bar * implement progressive loading design while blocking service map interaction during loading * adds filter that destination.address exists before fetching sample trace ids * reduce pairs of connections to 1 bi-directional connection with arrows on both ends of the edge * Optimize query; add update button * Allow user interaction after 5s, auto update in that time, otherwise show toast for user to update the map with button * Correctly reduce nodes/connections * - remove non-interactive state while loading - use cytoscape element definition types * - readability improvements to the ServiceMap component - only show the update map button toast after last request loads * addresses feedback for changes to the Cytoscape component * Add span.type/span.subtype do external nodes * PR feedback Co-authored-by: Dario Gieselaar <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]>

* upstream/master: (26 commits) Take page offset into account too (elastic#54567) [APM] Support error.{log,exception}.stacktrace.classname (elastic#54577) Np migration tsvb route validation (elastic#51850) [ML] MML calculator enhancements for multi-metric job wizard (elastic#54573) [SIEM] Fix Inspect query 'request timestamp' value changes when curso… (elastic#54223) Fix chromeless NP apps not using full page width (elastic#54550) Remove extraneous public import to prevent failing Kibana startup (elastic#54676) [Uptime] Temporarily skip flakey tests (elastic#54675) Skip failing uptime tests Create UI for alerting and actions plugin (elastic#48959) [dev/build/sass] build stylesheets for disabled plugins too (elastic#54654) [SIEM] Use bulk actions API when updating or deleting rules (elastic#54521) Support "Deprecated" label in advanced settings (elastic#54539) [Maps] add text halo color and width style properties (elastic#53827) Service Map Data API at Runtime (elastic#54027) [SIEM] Detection Engine Create Rule Design Review #1 (elastic#54442) Skip flaky test [Canvas] Enable Embeddable maps (elastic#53971) [SIEM][Detection Engine] Increases the number or rules you can view on a single page (elastic#54628) uiSettings - use validation field for image field maxSize (elastic#54522) ...

* [APM] Runtime service maps * Make nodes interactive * Don't use smaller range query on initial request * Address feedback from Ron * Get all services separately * Get single service as well * Query both transactions/spans for initial request * Optimize 'top' query for service maps * Use agent.name from scripted metric * adds basic loading overlay * filter out service map node self reference edges from being rendered * Make service map initial load time range configurable with `xpack.apm.serviceMapInitialTimeRange` default to last 1 hour in milliseconds * ensure destination.address is not missing in the composite agg when fetching sample trace ids * wip: added incremental data fetch & progress bar * implement progressive loading design while blocking service map interaction during loading * adds filter that destination.address exists before fetching sample trace ids * reduce pairs of connections to 1 bi-directional connection with arrows on both ends of the edge * Optimize query; add update button * Allow user interaction after 5s, auto update in that time, otherwise show toast for user to update the map with button * Correctly reduce nodes/connections * - remove non-interactive state while loading - use cytoscape element definition types * - readability improvements to the ServiceMap component - only show the update map button toast after last request loads * addresses feedback for changes to the Cytoscape component * Add span.type/span.subtype do external nodes * PR feedback Co-authored-by: Dario Gieselaar <[email protected]>

smith · 2020-01-23T21:37:47Z

Marking this as test plan done since we know it does what we need in its state behind the feature flag.

sorenlouv · 2020-02-19T22:48:04Z

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

+      >
+        <Controls />
+      </Cytoscape>
+    </LoadingOverlay>
  ) : (
    <PlatinumLicensePrompt />
  );


I stumbled upon this file, and before we release I think it would be great if we could clean it up. It very much resembles the POC state we've been in, but seeing we have settled on most aspects and are soon making a public release I think we should revisit this.

This component is currently responsible for: data fetching; polling logic, license checking; staleness checks + update button; custom loading indicator controls; custom environment handling; cytoscape specific behaviour - and probably other things I didn't spot.

sorenlouv · 2020-02-19T22:48:55Z

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

+        },
+        toastLifeTimeMs: 24 * 60 * 60 * 1000,
+        text: toMountPoint(
+          <EuiButton onClick={updateMap}>


If we are able to remove the update button completely I think a lot of complexity would vanish.

sorenlouv · 2020-02-19T22:49:35Z

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

+    if (renderedElements.current.length === 0) {
+      renderedElements.current = elements;
+      return;
+    }


Can you add a comment here - why are we returning early here?

sorenlouv · 2020-02-19T22:53:22Z

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

-          pathname: '/api/apm/service-map',
-          params: { query: { start, end } }
-        });
+        setIsLoading(true);


The loading state is already updated accordingly in callApmApi. Why the custom handling?

Correction: loading state is automatically handled if you use useFetcher instead of callApmApi. My guess is that callApmApi was chosen to make the fetch conditional. I have a gut feeling that this leads to procedural code with more edge cases. Instead I'd suggest using useFetcher and handle the conditional loading in the callback. This way we might be able to get rid of getNext since the useFetcher will automatically be invoked on every render

sorenlouv · 2020-02-19T22:59:09Z

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

+      ...uiFilters,
+      environment: undefined
+    }
+  });


If we need environment on the backend what about getting it from uiFilters.environment? I think rewriting uiFilters like this adds complexity that we could be without afaict.

Not sure this is related but the environment selector currently doesn't work. When changing environment the service map is unaffected and it's necessary to hard refresh to see the correct service map.

sorenlouv · 2020-02-19T23:06:36Z

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

+  const [responses, setResponses] = useState<ServiceMapAPIResponse[]>([]);
+  const [isLoading, setIsLoading] = useState(true);
+  const [percentageLoaded, setPercentageLoaded] = useState(0);
+  const [, _setUnusedState] = useState(false);


The number of useState, useRef, useMemo, useEffect and useCallback hooks in this file worries me a little. I'm not saying they are doing something wrong - but it points to a looot of things going on. Would be great if we can find some sensible abstractions that make this easier to comprehend and modify without fear.

sorenlouv · 2020-02-19T23:10:45Z

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

+      renderedElements.current = elements;
+      if (openToast.current) {
+        notifications.toasts.remove(openToast.current);
+      }


Do we still need to keep track of the currently open toast? Would be nice if we can remove this and just let the toast handle its own state.

sorenlouv · 2020-02-19T23:25:15Z

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

+          setIsLoading(false);
+
+          const shouldGetNext =
+            responses.length + 1 < MAX_REQUESTS && data.after;


would be great if this polling logic could be encapsulated

sorenlouv · 2020-02-19T23:42:15Z

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

+    loadServiceMaps();
+
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [params]);


If we really care about whether params change we should make it the dependency explicit and pass it through to getNext. But as I said in another comment: I think getNext forces us into an imperative mindset, whereas a hook based approach (useFetcher) will be more declarative.

formgeist mentioned this pull request Jan 7, 2020

[APM] Service map popover #53524

Merged

ogupte force-pushed the apm-48996-runtime-service-maps branch 2 times, most recently from 2ca87d1 to 8a987ea Compare January 9, 2020 18:18

smith reviewed Jan 9, 2020

View reviewed changes

ogupte force-pushed the apm-48996-runtime-service-maps branch from b4d9480 to df5cd36 Compare January 11, 2020 01:54

ogupte marked this pull request as ready for review January 11, 2020 02:08

ogupte requested a review from a team as a code owner January 11, 2020 02:08

ogupte added v7.6.0 release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support labels Jan 11, 2020

ogupte self-assigned this Jan 11, 2020

ogupte mentioned this pull request Jan 11, 2020

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

Closed

dgieselaar reviewed Jan 13, 2020

View reviewed changes

dgieselaar and others added 16 commits January 13, 2020 11:55

[APM] Runtime service maps

7c96d36

Make nodes interactive

19f8e47

Don't use smaller range query on initial request

61e5e99

Address feedback from Ron

4acf9b1

Get all services separately

f7b3cbe

Get single service as well

ea9d314

Query both transactions/spans for initial request

f99addc

Optimize 'top' query for service maps

d27c531

Use agent.name from scripted metric

14970d7

adds basic loading overlay

5f1df25

filter out service map node self reference edges from being rendered

8ca834a

Make service map initial load time range configurable with

08398d1

`xpack.apm.serviceMapInitialTimeRange` default to last 1 hour in milliseconds

ensure destination.address is not missing in the composite agg when

56d60c4

fetching sample trace ids

wip: added incremental data fetch & progress bar

141c4d2

implement progressive loading design while blocking service map inter…

7a0026a

…action during loading

adds filter that destination.address exists before fetching sample tr…

66fd5b9

…ace ids

ogupte and others added 4 commits January 13, 2020 11:55

- readability improvements to the ServiceMap component

5301125

- only show the update map button toast after last request loads

addresses feedback for changes to the Cytoscape component

b02b28c

Add span.type/span.subtype do external nodes

3ccffad

PR feedback

02cae50

ogupte force-pushed the apm-48996-runtime-service-maps branch from 0bcf5f8 to 02cae50 Compare January 13, 2020 19:56

smith approved these changes Jan 13, 2020

View reviewed changes

dgieselaar approved these changes Jan 13, 2020

View reviewed changes

ogupte merged commit b65710d into elastic:master Jan 13, 2020

ogupte mentioned this pull request Jan 13, 2020

[7.x] Service Map Data API at Runtime (#54027) #54655

Merged

formgeist mentioned this pull request Jan 14, 2020

[APM] Service Maps: Loading states on map initiation and while rendering #54246

Closed

ogupte removed their assignment Jan 15, 2020

ogupte added the apm-test-plan-7.6.0 label Jan 15, 2020

bmorelli25 mentioned this pull request Jan 16, 2020

[docs][APM] Documentation updates for 7.6 #54180

Closed

smith added the apm:test-plan-done Pull request that was successfully tested during the test plan label Jan 23, 2020

sorenlouv reviewed Feb 19, 2020

View reviewed changes

bmorelli25 mentioned this pull request Mar 24, 2020

[APM] Documentation updates for 7.7 #61104

Closed

4 tasks


		def lastLocation = basePath.size() > 0 ? basePath[basePath.size() - 1] : null;

		def currentLocation = new HashMap(service);

Service Map Data API at Runtime #54027

Service Map Data API at Runtime #54027

Conversation

ogupte commented Jan 6, 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

elasticmachine commented Jan 11, 2020

dgieselaar 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

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

smith left a comment

Choose a reason for hiding this comment

dgieselaar left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 13, 2020

💚 Build Succeeded

History

smith commented Jan 23, 2020

sorenlouv Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Feb 19, 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

sorenlouv Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

ogupte commented Jan 6, 2020 •

edited

Loading

sorenlouv Feb 19, 2020 •

edited

Loading

sorenlouv Feb 19, 2020 •

edited

Loading

sorenlouv Feb 19, 2020 •

edited

Loading

sorenlouv Feb 19, 2020 •

edited

Loading