Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Metrics UI] Performance improvements for Observability Homepage #70869

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions x-pack/plugins/infra/public/metrics_overview_fetchers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,7 @@ describe('Metrics UI Observability Homepage Functions', () => {
it('should return true when true', async () => {
const { core, mockedGetStartServices } = setup();
core.http.get.mockResolvedValue({
status: {
indexFields: [],
logIndicesExist: false,
metricIndicesExist: true,
},
hasData: true,
});
const hasData = createMetricsHasData(mockedGetStartServices);
const response = await hasData();
Expand All @@ -43,11 +39,7 @@ describe('Metrics UI Observability Homepage Functions', () => {
it('should return false when false', async () => {
const { core, mockedGetStartServices } = setup();
core.http.get.mockResolvedValue({
status: {
indexFields: [],
logIndicesExist: false,
metricIndicesExist: false,
},
hasData: false,
});
const hasData = createMetricsHasData(mockedGetStartServices);
const response = await hasData();
Expand Down
7 changes: 4 additions & 3 deletions x-pack/plugins/infra/public/metrics_overview_fetchers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,16 @@ import {
} from '../common/http_api/snapshot_api';
import { SnapshotMetricType } from '../common/inventory_models/types';
import { InfraClientCoreSetup } from './types';
import { SourceResponse } from '../common/http_api/source_api';

export const createMetricsHasData = (
getStartServices: InfraClientCoreSetup['getStartServices']
) => async () => {
const [coreServices] = await getStartServices();
const { http } = coreServices;
const results = await http.get<SourceResponse>('/api/metrics/source/default/metrics');
return results.status.metricIndicesExist;
const results = await http.get<{ hasData: boolean }>(
'/api/metrics/source/default/metrics/hasData'
);
return results.hasData;
};

export const average = (values: number[]) => (values.length ? sum(values) / values.length : 0);
Expand Down
52 changes: 41 additions & 11 deletions x-pack/plugins/infra/server/routes/source/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,21 @@ export const initSourceRoute = (libs: InfraBackendLibs) => {
try {
const { type, sourceId } = request.params;

const source = await libs.sources.getSourceConfiguration(
requestContext.core.savedObjects.client,
sourceId
);
const [source, logIndicesExist, metricIndicesExist, indexFields] = await Promise.all([
libs.sources.getSourceConfiguration(requestContext.core.savedObjects.client, sourceId),
libs.sourceStatus.hasLogIndices(requestContext, sourceId),
libs.sourceStatus.hasMetricIndices(requestContext, sourceId),
libs.fields.getFields(requestContext, sourceId, typeToInfraIndexType(type)),
]);

if (!source) {
return response.notFound();
}

const status = {
logIndicesExist: await libs.sourceStatus.hasLogIndices(requestContext, sourceId),
metricIndicesExist: await libs.sourceStatus.hasMetricIndices(requestContext, sourceId),
indexFields: await libs.fields.getFields(
requestContext,
sourceId,
typeToInfraIndexType(type)
),
logIndicesExist,
metricIndicesExist,
indexFields,
};

return response.ok({
Expand All @@ -65,4 +64,35 @@ export const initSourceRoute = (libs: InfraBackendLibs) => {
}
}
);

framework.registerRoute(
{
method: 'get',
path: '/api/metrics/source/{sourceId}/{type}/hasData',
Copy link
Member

Choose a reason for hiding this comment

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

Can we call this endpoint "indicesExist" or "hasIndices" ... something to distinguish it from checking for actual documents?

Copy link
Member

Choose a reason for hiding this comment

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

I also noticed that this returns "hasData: true" so long as just one of the configured indices exist. Maybe the endpoint should be named accordingly? "hasAnyConfiguredIndices" or "hasAnySourceIndices"?

Whatever we go with we should make sure the key in the response matches, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

This only returns true if the type passed in the URL has data. For example, if you don't have any logs data but you call /api/metrics/source/logs/hasData then it returns false. The {type} in the URL controls which indices it looks at.

Copy link
Member

@jasonrhodes jasonrhodes Jul 7, 2020

Choose a reason for hiding this comment

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

Does it check for actual documents though? I thought it was only checking if the indices exist?

Update: Maybe these two concepts aren't really that different in ES? If so, hasData is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like InfraElasticsearchSourceStatusAdapter:hasIndices() sends a query and checks to see if at least one shard has a document using the terminate_after: 1 to ensure it's as performant as possible.

 return await this.framework
      .callWithRequest(requestContext, 'search', {
        ignore_unavailable: true,
        allow_no_indices: true,
        index: indexNames,
        size: 0,
        terminate_after: 1,
      })
      .then(
        (response) => response._shards.total > 0,
        (err) => {
          if (err.status === 404) {
            return false;
          }
          throw err;
        }
      );

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. I saw that query and thought it was returning whether or not at least one shard returned having found the index, but if this is looking up at least one document, then hasData works here.

validate: {
params: schema.object({
sourceId: schema.string(),
type: schema.string(),
}),
},
},
async (requestContext, request, response) => {
try {
const { type, sourceId } = request.params;

const hasData =
type === 'metrics'
? await libs.sourceStatus.hasMetricIndices(requestContext, sourceId)
: await libs.sourceStatus.hasLogIndices(requestContext, sourceId);

return response.ok({
body: { hasData },
Copy link
Member

Choose a reason for hiding this comment

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

We should make this key match the endpoint whatever we decide.

});
} catch (error) {
return response.internalError({
body: error.message,
});
}
}
);
};
79 changes: 79 additions & 0 deletions x-pack/test/api_integration/apis/metrics_ui/http_source.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import expect from '@kbn/expect';

import { SourceResponse } from '../../../../plugins/infra/server/lib/sources';
import { FtrProviderContext } from '../../ftr_provider_context';

export default function ({ getService }: FtrProviderContext) {
const esArchiver = getService('esArchiver');
const supertest = getService('supertest');
const fetchSource = async (): Promise<SourceResponse | undefined> => {
const response = await supertest
.get('/api/metrics/source/default/metrics')
.set('kbn-xsrf', 'xxx')
.expect(200);
return response.body;
};
const fetchHasData = async (
type: 'logs' | 'metrics'
): Promise<{ hasData: boolean } | undefined> => {
const response = await supertest
.get(`/api/metrics/source/default/${type}/hasData`)
.set('kbn-xsrf', 'xxx')
.expect(200);
return response.body;
};

describe('Source API via HTTP', () => {
describe('8.0.0', () => {
before(() => esArchiver.load('infra/8.0.0/logs_and_metrics'));
after(() => esArchiver.unload('infra/8.0.0/logs_and_metrics'));
describe('/api/metrics/source/default/metrics', () => {
it('should just work', () => {
const resp = fetchSource();
return resp.then((data) => {
expect(data).to.have.property('source');
expect(data?.source.configuration.metricAlias).to.equal('metrics-*,metricbeat-*');
expect(data?.source.configuration.logAlias).to.equal(
'logs-*,filebeat-*,kibana_sample_data_logs*'
);
expect(data?.source.configuration.fields).to.eql({
container: 'container.id',
host: 'host.name',
message: ['message', '@message'],
pod: 'kubernetes.pod.uid',
tiebreaker: '_doc',
timestamp: '@timestamp',
});
expect(data).to.have.property('status');
expect(data?.status.metricIndicesExist).to.equal(true);
expect(data?.status.logIndicesExist).to.equal(true);
});
});
});
describe('/api/metrics/source/default/metrics/hasData', () => {
it('should just work', () => {
const resp = fetchHasData('metrics');
return resp.then((data) => {
expect(data).to.have.property('hasData');
expect(data?.hasData).to.be(true);
});
});
});
describe('/api/metrics/source/default/logs/hasData', () => {
it('should just work', () => {
const resp = fetchHasData('logs');
return resp.then((data) => {
expect(data).to.have.property('hasData');
expect(data?.hasData).to.be(true);
});
});
});
});
});
}
1 change: 1 addition & 0 deletions x-pack/test/api_integration/apis/metrics_ui/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ export default function ({ loadTestFile }) {
loadTestFile(require.resolve('./metrics_explorer'));
loadTestFile(require.resolve('./feature_controls'));
loadTestFile(require.resolve('./ip_to_hostname'));
loadTestFile(require.resolve('./http_source'));
});
}