[APM] Migrate tx latency chart and group stats to rollups/service metrics #153162

dgieselaar · 2023-03-12T15:30:04Z

Migrates the transaction latency chart and the transaction group statistics table to use service overview metrics/rollups when appropriate.

Part of #152693 and #146683 (the work is split up to prevent a huge PR).

elasticmachine · 2023-03-12T15:30:07Z

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

…-ref HEAD~1..HEAD --fix'

maryam-saeidi · 2023-03-14T10:06:52Z

x-pack/plugins/observability/public/utils/is_alert_details_enabled.ts

@@ -15,6 +15,7 @@ export const isAlertDetailsEnabledPerApp = (
  alert: TopAlert | null,
  config: ConfigSchema | null
 ): boolean => {
+  console.log(alert, config);


This is the only file change that we own, so after removing it there is no need for review from AO

thanks! missed this

…w-rollups-latency-group-stats

sorenlouv · 2023-03-20T08:31:45Z

x-pack/plugins/apm/common/document_type.ts

+export type ApmTransactionDocumentType =
+  | ApmDocumentType.TransactionMetric
+  | ApmDocumentType.TransactionEvent;


What about service transaction metric?

Suggested change

export type ApmTransactionDocumentType =

| ApmDocumentType.TransactionMetric

| ApmDocumentType.TransactionEvent;

export type ApmTransactionDocumentType =

| ApmDocumentType.ServiceTransactionMetric

| ApmDocumentType.TransactionMetric

| ApmDocumentType.TransactionEvent;

In case we add ApmDocumentType.ServiceTransactionMetric to this one, it becomes in ApmServiceTransactionDocumentType

yeah, it's about making sure that the metric contains certain data. ie, service-instance specific data (like host) will not be available on transaction metrics.

sorenlouv · 2023-03-21T15:42:26Z

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

 import {
  LatencyAggregationType,
  latencyAggregationTypeRt,
 } from '../../../common/latency_aggregation_types';
+import { getApmEventClient } from '../../lib/helpers/get_apm_event_client';
 import { getSearchTransactionsEvents } from '../../lib/helpers/transactions';


Not related to this PR but afaict getSearchTransactionsEvents should be renamed getSearchAggregatedTransactions. It was a little confusing debugging this when that function is named the exact opposite of what it actually does.

Suggested change

import { getSearchTransactionsEvents } from '../../lib/helpers/transactions';

import { getSearchAggregatedTransactions } from '../../lib/helpers/transactions';

Btw. I don't think we should use the term "aggregated transactions" anymore. I'd prefer to use the terms "transaction metrics" and "service transaction metrics" instead.

I think we'll eventually remove it anyway.

sorenlouv · 2023-03-21T15:48:00Z

x-pack/plugins/apm/public/components/shared/charts/latency_chart/index.tsx

@@ -65,6 +69,9 @@ export function LatencyChart({ height, kuery }: Props) {
    useTransactionLatencyChartsFetcher({
      kuery,
      environment,
+      transactionName:
+        'transactionName' in query ? query.transactionName : null,
+      latencyAggregationType: getLatencyAggregationType(latencyAggregationType),


Can't we use io-ts to validate that latencyAggregationType is indeed of type LatencyAggregationType?

Similar to what we do for sortDirection:

kibana/x-pack/plugins/apm/public/components/routing/service_detail/index.tsx

Line 156 in 894e83e

sortDirection: t.union([t.literal('asc'), t.literal('desc')]),

Suggested change

latencyAggregationType: getLatencyAggregationType(latencyAggregationType),

latencyAggregationType,

getLatencyAggregationType also returns a default value if it's undefined.

sorenlouv · 2023-03-21T16:03:54Z

x-pack/plugins/apm/public/components/alerting/ui_components/alert_details_app_section/index.tsx

              serviceName={serviceName}
+              start={historicalRange.start}
+              end={historicalRange.end}


Is this fix related to this this PR and the move to service transaction metrics?

it's a cosmetic change, it doesn't change the behaviour of the component.

sorenlouv · 2023-03-21T19:35:28Z

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


  const response = await apmEventClient.search(
    'get_service_transaction_groups',
    {
      apm: {
-        events: [
-          getProcessorEventForTransactions(searchAggregatedTransactions),
+        sources: [


I think we should eventually replace events with sources everywhere - even for the queries that don't support rollups or service metrics. It adds complexity to have to separate ways to define this.

agreed! I'm adding some additional support in a follow-up PR.

sorenlouv · 2023-03-21T19:38:12Z

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

@@ -99,9 +101,6 @@ export async function getServiceTransactionGroups({
                  ],
                },
              },
-              ...getDocumentTypeFilterForTransactions(
-                searchAggregatedTransactions
-              ),


This is no longer needed because you apply the filter in APMEventClient, right? We should aim to do this consistently everywhere. I'm sure someone will use events and then forget to add the filter explicitly because they expect it to be added automatically.

sorenlouv · 2023-03-21T20:11:16Z

x-pack/plugins/apm/public/components/shared/transactions_table/index.tsx

+    start,
+    end,
+    kuery,
+    numBuckets: 20,


Afaict numBuckets is used to calculate bucketSizeInSeconds - but this is not needed for the table. Can it be omitted?

Suggested change

numBuckets: 20,

numBuckets: 20,

The transaction group table has spark plots.

But this is for the main statistics (which doesn't include spark plots afaik)?

Ah good point. I think it should still be there though, 1 bucket might lead to a bigger error boundary (e.g. selecting the 1 hour bucket for 1.5 hours will lead to skewed statistics).

Hmm.. It still seems odd to specify number of buckets when the expected return value is a single metric number (as opposed to a timeseries) but no biggie.

x-pack/plugins/apm/public/components/shared/transactions_table/index.tsx

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

sorenlouv · 2023-03-21T20:53:55Z

x-pack/plugins/apm/public/context/time_range_metadata/time_range_metadata_context.tsx

+  return (
+    <TimeRangeMetadataContextProvider
+      uiSettings={uiSettings}
+      useSpanName={isOperationView}


Something still feels odd about this option (useSpanName). I don't understand why we have to special-case service destination metrics like this - isn't it just another source similar to transaction metrics and transaction events?

I think the response of GET /internal/apm/time_range_metadata shows the inconsistency:

{ "isUsingServiceDestinationMetrics": true, "sources": [ { "documentType": "serviceTransactionMetric", "rollupInterval": "1m", "hasDocs": true }, { "documentType": "serviceTransactionMetric", "rollupInterval": "10m", "hasDocs": true }, { "documentType": "serviceTransactionMetric", "rollupInterval": "60m", "hasDocs": true }, { "documentType": "transactionMetric", "rollupInterval": "1m", "hasDocs": true }, { "documentType": "transactionMetric", "rollupInterval": "10m", "hasDocs": true }, { "documentType": "transactionMetric", "rollupInterval": "60m", "hasDocs": true }, { "documentType": "transactionEvent", "rollupInterval": "none", "hasDocs": true } ] }

Shouldn't the service destination metrics be part of sources, like:

{ "sources": [ { "documentType": "serviceDestinationMetrics", "rollupInterval": "none", "hasDocs": true }, { "documentType": "spanEvents", "rollupInterval": "none", "hasDocs": true }, // ... ] }

Apart from making things more consistent, we're now able to have views where one component uses serviceDestinationMetrics and another uses spanEvents similar to what we do with transactions.

In earlier versions span.name was not available on span metrics. That was a fairly recent change. But we have a couple of views that were added after, and require span.name to be present. If span.name is not available on span metrics, we fall back to span events.

Then what about something like this?

{ "sources": [ { "documentType": "spanMetricsWithName", "rollupInterval": "none", "hasDocs": false }, { "documentType": "spanMetrics", "rollupInterval": "none", "hasDocs": true }, { "documentType": "spanEvents", "rollupInterval": "none", "hasDocs": true }, // ... ] }

I've considered that, also in the context of transaction metrics with transaction.duration.summary, but I don't think different document types for schema changes will scale. I would rather return some metadata with the document type, e.g. a version or a meta property. But I will tackle that when I implement support for transaction.duration.summary on tx metrics (which might not include changes to service destination metrics as it makes sense to tackle that separately).

I would rather return some metadata with the document type, e.g. a version or a meta property.

That makes sense. Not a blocker but it would be cleaner with a "unified" model for this. The useSpanName seems like an afterthought.

sorenlouv · 2023-03-21T21:03:51Z

GET /internal/apm/time_range_metadata doesn't take serviceName as a param. This means that if any service has service transaction metrics, it is expected that all other services will have it too.
This is probably a correct assumption in most cases but we should double check that we are not causing too much disruption with this change (eg. showing empty states for services without service transaction metrics). We could add a serviceName param to ensure it re-runs for every service which wouldn't have too big of a perf impact.

yngrdyn · 2023-03-22T11:48:14Z

packages/kbn-typed-react-router-config/src/router_provider.tsx

@@ -18,7 +18,7 @@ export function RouterProvider({
 }: {
  router: Router<RouteMap>;
  history: History;
-  children: React.ReactNode;
+  children?: React.ReactNode;


Is this intended?

Yeah, no need for it to be required.

yngrdyn · 2023-03-22T11:50:34Z

x-pack/plugins/apm/common/document_type.ts

+export type ApmTransactionDocumentType =
+  | ApmDocumentType.TransactionMetric
+  | ApmDocumentType.TransactionEvent;


In case we add ApmDocumentType.ServiceTransactionMetric to this one, it becomes in ApmServiceTransactionDocumentType

…/index.tsx Co-authored-by: Søren Louv-Jansen <sorenlouv@gmail.com>

…ex.tsx Co-authored-by: Søren Louv-Jansen <sorenlouv@gmail.com>

dgieselaar · 2023-03-22T16:16:09Z

@sqren:

GET /internal/apm/time_range_metadata doesn't take serviceName as a param. This means that if any service has service transaction metrics, it is expected that all other services will have it too.
This is probably a correct assumption in most cases but we should double check that we are not causing too much disruption with this change (eg. showing empty states for services without service transaction metrics). We could add a serviceName param to ensure it re-runs for every service which wouldn't have too big of a perf impact.

This has always been the behaviour, it's not changing.

…w-rollups-latency-group-stats

….com:dgieselaar/kibana into service-overview-rollups-latency-group-stats

kibana-ci · 2023-03-23T10:16:58Z

💚 Build Succeeded

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.8MB	3.8MB	+1.9KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💔 Build #114731 failed ae59097
💛 Build #113963 was flaky f75200a
💛 Build #113779 was flaky c41f5d5
💔 Build #113545 failed 2c4d36c

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

Migrate tx latency chart

01bc00b

dgieselaar added release_note:enhancement Team:APM All issues that need APM UI Team support v8.8.0 labels Mar 12, 2023

dgieselaar requested review from a team as code owners March 12, 2023 15:30

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

2c4d36c

…-ref HEAD~1..HEAD --fix'

maryam-saeidi reviewed Mar 14, 2023

View reviewed changes

dgieselaar added 3 commits March 15, 2023 16:40

Remove console.log statement

aafd31d

Merge branch 'main' of github.com:elastic/kibana into service-overvie…

c41f5d5

…w-rollups-latency-group-stats

Merge branch 'main' of github.com:elastic/kibana into service-overvie…

f75200a

…w-rollups-latency-group-stats

sorenlouv reviewed Mar 21, 2023

View reviewed changes

yngrdyn reviewed Mar 22, 2023

View reviewed changes

dgieselaar and others added 2 commits March 22, 2023 17:13

Update x-pack/plugins/apm/public/components/shared/transactions_table…

c792043

…/index.tsx Co-authored-by: Søren Louv-Jansen <sorenlouv@gmail.com>

Update x-pack/plugins/apm/public/components/app/service_inventory/ind…

074d14d

…ex.tsx Co-authored-by: Søren Louv-Jansen <sorenlouv@gmail.com>

dgieselaar added 4 commits March 22, 2023 17:18

Merge branch 'main' into service-overview-rollups-latency-group-stats

ae59097

Merge branch 'main' of github.com:elastic/kibana into service-overvie…

115e2f6

…w-rollups-latency-group-stats

Merge branch 'service-overview-rollups-latency-group-stats' of github…

b4f5d26

….com:dgieselaar/kibana into service-overview-rollups-latency-group-stats

Fix type errors

40c4721

sorenlouv approved these changes Mar 23, 2023

View reviewed changes

dgieselaar merged commit 97bca50 into elastic:main Mar 23, 2023

dgieselaar deleted the service-overview-rollups-latency-group-stats branch March 23, 2023 10:30

kibanamachine added the backport:skip This commit does not require backporting label Mar 23, 2023

sorenlouv mentioned this pull request May 9, 2023

[APM] Meta: Service transaction metrics, rollups and transaction.duration.summary field everywhere #157227

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Migrate tx latency chart and group stats to rollups/service metrics #153162

[APM] Migrate tx latency chart and group stats to rollups/service metrics #153162

dgieselaar commented Mar 12, 2023

elasticmachine commented Mar 12, 2023

maryam-saeidi Mar 14, 2023

dgieselaar Mar 15, 2023

sorenlouv Mar 20, 2023

yngrdyn Mar 22, 2023

dgieselaar Mar 22, 2023

sorenlouv Mar 21, 2023

dgieselaar Mar 22, 2023

sorenlouv Mar 21, 2023

dgieselaar Mar 22, 2023

sorenlouv Mar 21, 2023

dgieselaar Mar 22, 2023

sorenlouv Mar 21, 2023

dgieselaar Mar 22, 2023

sorenlouv Mar 21, 2023

dgieselaar Mar 22, 2023

sorenlouv Mar 21, 2023

dgieselaar Mar 22, 2023

sorenlouv Mar 22, 2023 •

edited

Loading

dgieselaar Mar 22, 2023

sorenlouv Mar 23, 2023

sorenlouv Mar 21, 2023

dgieselaar Mar 22, 2023

sorenlouv Mar 23, 2023

dgieselaar Mar 23, 2023

sorenlouv Mar 23, 2023

sorenlouv commented Mar 21, 2023 •

edited

Loading

yngrdyn Mar 22, 2023

dgieselaar Mar 22, 2023

yngrdyn Mar 22, 2023

dgieselaar commented Mar 22, 2023

kibana-ci commented Mar 23, 2023

ESLint disabled line counts

Total ESLint disabled count

	import { getSearchTransactionsEvents } from '../../lib/helpers/transactions';
	import { getSearchAggregatedTransactions } from '../../lib/helpers/transactions';

	latencyAggregationType: getLatencyAggregationType(latencyAggregationType),
	latencyAggregationType,

[APM] Migrate tx latency chart and group stats to rollups/service metrics #153162

[APM] Migrate tx latency chart and group stats to rollups/service metrics #153162

Conversation

dgieselaar commented Mar 12, 2023

elasticmachine commented Mar 12, 2023

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

sorenlouv Mar 22, 2023 • 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

sorenlouv commented Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar commented Mar 22, 2023

kibana-ci commented Mar 23, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

sorenlouv Mar 22, 2023 •

edited

Loading

sorenlouv commented Mar 21, 2023 •

edited

Loading