[APM] Service groups - dynamic query refresh by gbamparop · Pull Request #140406 · elastic/kibana

gbamparop · 2022-09-09T13:44:00Z

Create migration for service group saved objects, removing service names
Dynamic refresh of services in service groups
Compute the service count for a group at runtime (hardcoded to last 24 hours)
Users will be restricted to filtering by dimensions available in metric documents
Dimensions: agent.name, service.name, service.language.name, service.environment, labels.
Update x-pack/plugins/apm/server/routes/services/get_services/get_sorted_and_filtered_services.ts to work with dynamic refresh
Add tests for saved object migrations

This is a draft PR and all the changes should be revisited. observability:apmServiceGroupMaxNumberOfServices is sometimes used outside of the context of service groups while in other queries max service count is hardcoded. We could rename the setting and use it in all service queries.

Closes #133112

…estions

elasticmachine · 2022-09-19T20:41:08Z

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

cauemarcondes

Some comments on the server side

x-pack/plugins/apm/server/saved_objects/apm_service_groups.ts

x-pack/plugins/apm/server/routes/service_map/route.ts

x-pack/plugins/apm/server/routes/service_groups/route.ts

x-pack/plugins/apm/server/lib/service_group_query.ts

x-pack/plugins/apm/public/components/app/service_groups/service_groups_list/index.tsx

kibana-ci · 2022-09-20T03:42:01Z

💚 Build Succeeded

Buildkite Build
Commit: e8ba4a0

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	3.0MB	3.0MB	+334.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id	before	after	diff
`apm-service-group`	6	5	-1

History

💔 Build #73707 failed 9916a7d
💔 Build #73648 failed c95ec06
💔 Build #73378 failed e3cbff6bc613169ab8853709545e1ea90c038670
💔 Build #70846 failed c55a43141e520111ceea36111268a61371fbabf5

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

cc @ogupte

sorenlouv · 2022-09-20T07:22:36Z

x-pack/plugins/apm/public/components/app/service_groups/service_group_save/select_services.tsx

+                      const {
+                        field: {
+                          spec: { name: fieldName },
+                        },
+                      } = querySuggestion;


nit: this is 100% personal preference but my brain always struggle a bit with nested destructurings. Feel free to leave as-is if you prefer that.

Suggested change

const {

field: {

spec: { name: fieldName },

},

} = querySuggestion;

const fieldName = querySuggestion.field.spec.name;

sorenlouv · 2022-09-20T07:29:20Z

x-pack/plugins/apm/public/components/app/service_groups/service_groups_list/index.tsx

+  const { start, end } = useMemo(
+    () =>
+      getDateRange({
+        rangeFrom: 'now-24h',
+        rangeTo: 'now',
+      }),
+    []
+  );


This is on the service group overview page (aka the page listing all the service groups), right?

@gbamparop and I talked about that we didn't currently have a timepicker, and we didn't see any need to add any. It also means that the lookback time-period for calculating the service counts is hardcoded to 24 hours.

I think this is a fine compromise between simplicity and functionality. But we should remember to flag this to product (and possible docs to document it).

Correct, this is on the service group overview page.

sorenlouv · 2022-09-20T07:36:55Z

x-pack/plugins/apm/server/routes/service_groups/route.ts

+        async ({
+          id,
+          kuery,
+        }): Promise<{ id: string; servicesCount: number }> => {


Is this type annotation necessary ?

It seems that it isn't, we can remove it

sorenlouv · 2022-09-20T07:38:39Z

x-pack/plugins/apm/server/routes/service_groups/get_services_counts.ts

+}: {
+  setup: Setup;
+  kuery: string;
+  maxNumberOfServices: number;


Is maxNumberOfServices unused here?

It is unused, we can add a size (service inventory and service map already have max size specified).

dgieselaar · 2022-09-20T08:01:44Z

x-pack/plugins/apm/public/components/app/service_groups/service_group_save/select_services.tsx

 const MODAL_HEADER_HEIGHT = 122;
 const MODAL_FOOTER_HEIGHT = 80;

+const suggestedFieldsWhitelist = [


Can you use allowlist here?

dgieselaar · 2022-09-20T08:09:55Z

x-pack/plugins/apm/server/routes/service_groups/route.ts

      t.partial({
        serviceGroupId: t.string,
      }),
+      t.undefined,


Do you actually want t.union([ t.type(), t.partial() ]) here?

dgieselaar · 2022-09-20T08:11:24Z

x-pack/plugins/apm/server/routes/service_map/route.ts

-      ...(serviceGroup?.serviceNames ?? []),
-    ];
+    const serviceNames = compact([serviceName]);



What happens if this is an empty array? Does it return a service map for all services or no service map at all?

It returns a service map for all services. I think that was there before to handle service-specific service maps

dgieselaar · 2022-09-20T08:13:05Z

x-pack/plugins/apm/server/routes/service_map/route.ts

      end,
      maxNumberOfServices,
+      serviceGroup,
    });


Why do we pass both serviceNames and serviceGroup?

Service name is used when we are in the context of a service. We could rename it to serviceName. Service group is passed to apply a kuery filter for the group

sorenlouv · 2022-09-20T08:27:27Z

x-pack/plugins/apm/server/saved_objects/apm_service_groups.ts

+  color: string;
+}
+
+const migrateApmServiceGroups850: SavedObjectMigrationFn<


Is that a convention to put the version in the end like that? Perhaps we can make it a little clearer:

Suggested change

const migrateApmServiceGroups850: SavedObjectMigrationFn<

const migrateApmServiceGroups_v8_5_0: SavedObjectMigrationFn<

Not sure if this is a convention but It's aligned with https://docs.elastic.dev/kibana-dev-docs/tutorials/saved-objects#writing-migrations

sorenlouv · 2022-09-20T08:27:51Z

x-pack/plugins/apm/server/saved_objects/apm_service_groups.ts

+}
+
+const migrateApmServiceGroups850: SavedObjectMigrationFn<
+  ApmServiceGroupsPre850,


Suggested change

ApmServiceGroupsPre850,

ApmServiceGroupsPre_v8_5_0,

I don't think we'll hit v85.0 anytime soon but by using separators we at least don't run the risk of confusion in the future :D

dgieselaar · 2022-09-20T10:18:18Z

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

    }),
    serviceGroup
-      ? getServiceNamesFromServiceGroup(serviceGroup)
+      ? getServiceNamesFromServiceGroup({


This isn't useful anymore. The intent of this function is to quickly get a list of sorted and filtered services, but as this now aggregates over all the data, including raw transactions and spans, it is likely to be slower than the request that gets the actual statistics. We should remove it, I think.

That's true, we wouldn't get the benefit of terms enum. Removing it though could result in preloaded service names being removed once we get the "final" response right?

We probably shouldn't preload at all if a service group is selected. So, simply returning an empty array here might suffice. But I don't know how it specifically works. terms_enum doesn't make sense if a service group is selected, we shouldn't use it at all in that case (I think that's already happening today).

dgieselaar · 2022-09-20T10:24:42Z

x-pack/plugins/apm/server/routes/service_groups/route.ts

+          id,
+          kuery,
+        }): Promise<{ id: string; servicesCount: number }> => {
+          const servicesCount = await getServicesCounts({


have you compared performance of a single request w/ filter aggregations vs multiple requests?

Good point, I haven't checked filter aggregations, @ogupte have you? If not, we could have a look

yeah we should probably fix this since it can get out of hand with many service groups.

Some findings on this comparison in a related issue (#141242): #141242 (comment)

gbamparop added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Sep 9, 2022

ogupte force-pushed the service-groups-dynamic-refresh branch from c55a431 to e3cbff6 Compare September 19, 2022 13:30

ogupte self-assigned this Sep 19, 2022

gbamparop and others added 5 commits September 19, 2022 16:40

Service groups - dynamic query refresh

6469b13

fixes bug with group save API which omits the query params

1f0ad50

don't render 0 services if no services count is given

7a201df

adds support for dynamic refresh in getSortedAndFilteredServices

8cd4942

only include a whitelisted set of metric doc fields in kuery bar sugg…

c95ec06

…estions

ogupte force-pushed the service-groups-dynamic-refresh branch from eef2aa5 to c95ec06 Compare September 19, 2022 20:40

ogupte added v8.5.0 release_note:skip Skip the PR/issue when compiling release notes labels Sep 19, 2022

ogupte marked this pull request as ready for review September 19, 2022 20:41

ogupte requested a review from a team September 19, 2022 20:41

cauemarcondes reviewed Sep 19, 2022

View reviewed changes

sorenlouv reviewed Sep 19, 2022

View reviewed changes

x-pack/plugins/apm/public/components/app/service_groups/service_groups_list/index.tsx Outdated Show resolved Hide resolved

ogupte added 4 commits September 19, 2022 21:56

Merge branch 'main' into service-groups-dynamic-refresh

a0275e5

clean up unused i18n translation

d749c1a

address PR feedback

9916a7d

fixes bug with a bad translation variable reference

e8ba4a0

ogupte enabled auto-merge (squash) September 20, 2022 02:49

sorenlouv reviewed Sep 20, 2022

View reviewed changes

dgieselaar reviewed Sep 20, 2022

View reviewed changes

sorenlouv reviewed Sep 20, 2022

View reviewed changes

sorenlouv approved these changes Sep 20, 2022

View reviewed changes

ogupte merged commit 256f2e7 into elastic:main Sep 20, 2022

kibanamachine added the backport:skip This PR does not require backporting label Sep 20, 2022

dgieselaar reviewed Sep 20, 2022

View reviewed changes

ogupte mentioned this pull request Sep 21, 2022

[APM] Performance impacted with large number of service groups #141242

Closed

4 tasks

sorenlouv mentioned this pull request Oct 6, 2022

[APM] Add service metrics (global labels) support to Service Groups #142195

Closed

ogupte mentioned this pull request Oct 24, 2022

[APM] Support specific fields when creating service groups (#142201) #143881

Merged

3 tasks

gbamparop mentioned this pull request Dec 19, 2022

[APM] Evaluate technical design of Service Groups #128634

Closed

	const migrateApmServiceGroups850: SavedObjectMigrationFn<
	const migrateApmServiceGroups_v8_5_0: SavedObjectMigrationFn<

Conversation

gbamparop commented Sep 9, 2022 • edited by ogupte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 19, 2022

Uh oh!

cauemarcondes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibana-ci commented Sep 20, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

Saved Objects .kibana field count

History

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!

sorenlouv Sep 20, 2022 • 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!

gbamparop Sep 20, 2022 • 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!

gbamparop Sep 20, 2022 • 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!

dgieselaar Sep 20, 2022 • 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!

Reviewers

Assignees

Labels

gbamparop commented Sep 9, 2022 •

edited by ogupte

Loading

sorenlouv Sep 20, 2022 •

edited

Loading

gbamparop Sep 20, 2022 •

edited

Loading

gbamparop Sep 20, 2022 •

edited

Loading

dgieselaar Sep 20, 2022 •

edited

Loading