Remove dashboard performance measurement gap

[drewdaemon]

Summary

This change corrects the accuracy of two related EBT metrics.

• kbnLoad — supposed to end the moment core gives over control to the application
  • The problem: the mark was being set after the intended moment because it was not accounting for the cost of an observable chain
• dashboard_duration supposed to start the moment the dashboard app is mounted
  • The problem: the timing wasn't accounting for the full body of the application's mount method, leaving some work unaccounted for (~150ms for a fresh load)

Before

notice the gap in the measures

After

perfect stacking

[drewdaemon]

@elasticmachine merge upstream

[jloleysens]

This approach makes sense to me! I'd like to request @afharo 's sanity check so leaving this as a comment and one minor change request. Nice work!

EDIT: I'll also ask @maryam-saeidi

[jloleysens]

Can we use the const KBN_LOAD_MARKS and LOAD_FIRST_NAV here? Would make it clear that these performance.mark calls are linked.

[drewdaemon]

I tried, but it created a circular dependency since the KBN_LOAD_MARKS lives in the root package. If you want, I could move it to the application package. LMK

[maryam-saeidi]

From my side, these changes LGTM!

[drewdaemon]

@elasticmachine merge upstream

[drewdaemon]

@elasticmachine merge upstream

[elasticmachine]

There are no new commits on the base branch.

[nreese]

This only works for the first dashboard loaded in a session. What happens if the user loads another dashboard? Then that dashboard load time would be pegged to application mount. Should dashboard_app_mount be cleared after the first load and all subsequent loads use performance.now()?

[drewdaemon]

Thanks for raising. This mount method is called every time the dashboard app is loaded, but I see it includes the listing page, too, so I'll definitely need to adjust.

[drewdaemon]

Updated in ff7ced4

[nreese]

Can you move these expects into a new test case. This test case is "should get initialState from saved object" so it does not make sense to assert performance tracking. I would recommend a new test case with the name "should start performance tracking on load" or something like that.

[drewdaemon]

How about the existing reports initial and subsequent loads? Clearing the mark feels like part of that because it should happen whenever we report a load.

[drewdaemon]

Updated in 74ec9b6

LMK if you want something else

[nreese]

74ec9b6 made changes in src/platform/plugins/shared/dashboard/public/dashboard_api/performance/query_performance_tracking.test.ts. This comment is about src/platform/plugins/shared/dashboard/public/dashboard_api/load_dashboard_api/load_dashboard_api.test.ts

[drewdaemon]

My bad. Okay, check 79aa7c4

[nreese]

There are still some artifacts from your first changes and the test does not makes sense in the initialState describe block. How about you try the below

/*
 * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
 * or more contributor license agreements. Licensed under the "Elastic License
 * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
 * Public License v 1"; you may not use this file except in compliance with, at
 * your election, the "Elastic License 2.0", the "GNU Affero General Public
 * License v3.0 only", or the "Server Side Public License, v 1".
 */

import { DEFAULT_DASHBOARD_STATE } from '../default_dashboard_state';
import { loadDashboardApi } from './load_dashboard_api';

jest.mock('../performance/query_performance_tracking', () => {
  return {
    startQueryPerformanceTracking: jest.fn(),
  };
});

import { startQueryPerformanceTracking } from '../performance/query_performance_tracking';
import { DASHBOARD_DURATION_START_MARK } from '../performance/dashboard_duration_start_mark';

jest.mock('@kbn/content-management-content-insights-public', () => {
  class ContentInsightsClientMock {
    track() {}
  }
  return {
    ContentInsightsClient: ContentInsightsClientMock,
  };
});

jest.mock('../../dashboard_client', () => {
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  const defaultState = require('../default_dashboard_state');
  return {
    dashboardClient: {
      get: jest.fn().mockResolvedValue({
        data: { ...defaultState.DEFAULT_DASHBOARD_STATE },
      }),
    },
  };
});

const lastSavedQuery = { query: 'memory:>220000', language: 'kuery' };

describe('loadDashboardApi', () => {
  const getDashboardApiMock = jest.fn();

  beforeEach(() => {
    // eslint-disable-next-line @typescript-eslint/no-var-requires
    require('../get_dashboard_api').getDashboardApi = getDashboardApiMock;
    getDashboardApiMock.mockReturnValue({
      api: {},
      cleanUp: jest.fn(),
      internalApi: {},
    });

    // eslint-disable-next-line @typescript-eslint/no-var-requires
    require('../../services/dashboard_backup_service').getDashboardBackupService = () => ({
      getState: () => ({
        query: lastSavedQuery,
      }),
    });

    window.performance.getEntriesByName = jest.fn().mockReturnValue([
      {
        startTime: 12345,
      },
    ]);
  });

  afterEach(() => {
    jest.resetAllMocks();
  });

  describe('initialState', () => {
    test('should get initialState from saved object', async () => {
      await loadDashboardApi({
        getCreationOptions: async () => ({
          useSessionStorageIntegration: false,
        }),
        savedObjectId: '12345',
      });
      expect(getDashboardApiMock).toHaveBeenCalled();
      // @ts-ignore
      expect(getDashboardApiMock.mock.calls[0][0].initialState).toEqual(DEFAULT_DASHBOARD_STATE);
    });

    test('should overwrite saved object state with unsaved state', async () => {
      await loadDashboardApi({
        getCreationOptions: async () => ({
          useSessionStorageIntegration: true,
        }),
        savedObjectId: '12345',
      });
      expect(getDashboardApiMock).toHaveBeenCalled();
      // @ts-ignore
      expect(getDashboardApiMock.mock.calls[0][0].initialState).toEqual({
        ...DEFAULT_DASHBOARD_STATE,
        query: lastSavedQuery,
      });
    });

    // dashboard app passes URL state as override state
    test('should overwrite saved object state and unsaved state with override state', async () => {
      const queryFromUrl = { query: 'memory:>5000', language: 'kuery' };
      await loadDashboardApi({
        getCreationOptions: async () => ({
          useSessionStorageIntegration: true,
          getInitialInput: () => ({
            query: queryFromUrl,
          }),
        }),
        savedObjectId: '12345',
      });
      expect(getDashboardApiMock).toHaveBeenCalled();
      // @ts-ignore
      expect(getDashboardApiMock.mock.calls[0][0].initialState).toEqual({
        ...DEFAULT_DASHBOARD_STATE,
        query: queryFromUrl,
      });
    });
  });

  describe('performance monitoring', () => {
    test('should start performance tracking on load', async () => {
      await loadDashboardApi({
        getCreationOptions: async () => ({
          useSessionStorageIntegration: false,
        }),
        savedObjectId: '12345',
      });

      expect(window.performance.getEntriesByName).toHaveBeenCalledWith(
        DASHBOARD_DURATION_START_MARK,
        'mark'
      );
      expect(startQueryPerformanceTracking).toHaveBeenCalledWith(expect.any(Object), {
        firstLoad: true,
        creationStartTime: 12345,
      });
    });
  });
});

[drewdaemon]

2f1a00b

[drewdaemon]

@elasticmachine merge upstream

[nreese]

kibana-presentation changes LGTM - thanks for updating the metrics to be more accurate.
code review only

[elasticmachine]

💔 Build Failed

• Buildkite Build
• Commit: fda220f

Failed CI Steps

• FTR Configs #40
• FTR Configs #40
• FTR Configs #48
• FTR Configs #48

Test Failures

• [job] [logs] FTR Configs #40 / Entity Analytics - Privilege Monitoring @ess @serverless @skipInServerlessMKI Entity Privilege Monitoring APIs privileges checks should return has_all_required false for user with no privileges
• [job] [logs] FTR Configs #48 / Entity Analytics - Privilege Monitoring @ess @serverless @skipInServerlessMKI Entity Privilege Monitoring APIs privileges checks should return has_all_required false for user with no privileges
• [job] [logs] FTR Configs #40 / Entity Analytics - Privilege Monitoring @ess @serverless @skipInServerlessMKI Entity Privilege Monitoring APIs privileges checks should return has_all_required false for user with no privileges
• [job] [logs] FTR Configs #48 / Entity Analytics - Privilege Monitoring @ess @serverless @skipInServerlessMKI Entity Privilege Monitoring APIs privileges checks should return has_all_required false for user with no privileges

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
dashboard	945	946	+1

Async chunks

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

id	before	after	diff
dashboard	673.3KB	673.6KB	+260.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
core	507.4KB	507.4KB	+18.0B
dashboard	17.6KB	17.7KB	+117.0B
total			+135.0B

History

• 💔 Build #363375 failed 74ec9b6
• 💔 Build #362951 failed 4844922
• 💚 Build #362929 succeeded a2ea7cc
• 💔 Build #362848 failed e8f99b2

[drewdaemon]

Thanks for your help, @nreese !