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

[Rule Registry] Additional error logging #136225

Merged
merged 8 commits into from
Jul 22, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
*/

import { left, right } from 'fp-ts/lib/Either';
import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { elasticsearchClientMock } from '@kbn/core/server/elasticsearch/client/mocks';
import { RuleDataClient, RuleDataClientConstructorOptions, WaitResult } from './rule_data_client';
import { IndexInfo } from '../rule_data_plugin_service/index_info';
import { Dataset, RuleDataWriterInitializationError } from '..';
Expand All @@ -15,7 +18,7 @@ import { IndexPatternsFetcher } from '@kbn/data-plugin/server';
import { createNoMatchingIndicesError } from '@kbn/data-views-plugin/server/fetcher/lib/errors';
import { ElasticsearchClient } from '@kbn/core/server';

const mockLogger = loggingSystemMock.create().get();
const logger: ReturnType<typeof loggingSystemMock.createLogger> = loggingSystemMock.createLogger();
const scopedClusterClient = elasticsearchServiceMock.createScopedClusterClient().asInternalUser;
const mockResourceInstaller = resourceInstallerMock.create();

Expand Down Expand Up @@ -52,7 +55,7 @@ function getRuleDataClientOptions({
waitUntilReadyForReading ?? Promise.resolve(right(scopedClusterClient) as WaitResult),
waitUntilReadyForWriting:
waitUntilReadyForWriting ?? Promise.resolve(right(scopedClusterClient) as WaitResult),
logger: mockLogger,
logger,
};
}

Expand All @@ -71,15 +74,12 @@ describe('RuleDataClient', () => {
});

describe('getReader()', () => {
beforeAll(() => {
beforeEach(() => {
jest.resetAllMocks();
getFieldsForWildcardMock.mockResolvedValue(['foo']);
IndexPatternsFetcher.prototype.getFieldsForWildcard = getFieldsForWildcardMock;
});

beforeEach(() => {
getFieldsForWildcardMock.mockClear();
});

afterAll(() => {
getFieldsForWildcardMock.mockRestore();
});
Expand Down Expand Up @@ -116,6 +116,10 @@ describe('RuleDataClient', () => {
body: query,
})
).rejects.toThrowErrorMatchingInlineSnapshot(`"something went wrong!"`);

expect(logger.error).toHaveBeenCalledWith(
`Error performing search in RuleDataClient - something went wrong!`
);
});

test('waits until cluster client is ready before getDynamicIndexPattern', async () => {
Expand Down Expand Up @@ -143,6 +147,10 @@ describe('RuleDataClient', () => {
await expect(reader.getDynamicIndexPattern()).rejects.toThrowErrorMatchingInlineSnapshot(
`"something went wrong!"`
);

expect(logger.error).toHaveBeenCalledWith(
`Error fetching index patterns in RuleDataClient - something went wrong!`
);
});

test('correct handles no_matching_indices errors from getFieldsForWildcard', async () => {
Expand All @@ -155,6 +163,8 @@ describe('RuleDataClient', () => {
timeFieldName: '@timestamp',
title: '.alerts-observability.apm.alerts*',
});

expect(logger.error).not.toHaveBeenCalled();
});

test('handles errors getting cluster client', async () => {
Expand Down Expand Up @@ -193,7 +203,7 @@ describe('RuleDataClient', () => {
await expect(() => ruleDataClient.getWriter()).rejects.toThrowErrorMatchingInlineSnapshot(
`"Rule registry writing is disabled. Make sure that \\"xpack.ruleRegistry.write.enabled\\" configuration is not set to false and \\"observability.apm\\" is not disabled in \\"xpack.ruleRegistry.write.disabledRegistrationContexts\\" within \\"kibana.yml\\"."`
);
expect(mockLogger.debug).toHaveBeenCalledWith(
expect(logger.debug).toHaveBeenCalledWith(
`Writing is disabled, bulk() will not write any data.`
);
});
Expand All @@ -210,15 +220,15 @@ describe('RuleDataClient', () => {
await expect(() => ruleDataClient.getWriter()).rejects.toThrowErrorMatchingInlineSnapshot(
`"There has been a catastrophic error trying to install index level resources for the following registration context: observability.apm. This may have been due to a non-additive change to the mappings, removal and type changes are not permitted. Full error: Error: could not get cluster client"`
);
expect(mockLogger.error).toHaveBeenNthCalledWith(
expect(logger.error).toHaveBeenNthCalledWith(
1,
new RuleDataWriterInitializationError(
'index',
'observability.apm',
new Error('could not get cluster client')
)
);
expect(mockLogger.error).toHaveBeenNthCalledWith(
expect(logger.error).toHaveBeenNthCalledWith(
2,
`The writer for the Rule Data Client for the observability.apm registration context was not initialized properly, bulk() cannot continue, and writing will be disabled.`
);
Expand All @@ -228,7 +238,7 @@ describe('RuleDataClient', () => {
await expect(() => ruleDataClient.getWriter()).rejects.toThrowErrorMatchingInlineSnapshot(
`"Rule registry writing is disabled due to an error during Rule Data Client initialization."`
);
expect(mockLogger.debug).toHaveBeenCalledWith(
expect(logger.debug).toHaveBeenCalledWith(
`Writing is disabled, bulk() will not write any data.`
);
});
Expand All @@ -242,15 +252,15 @@ describe('RuleDataClient', () => {
await expect(() => ruleDataClient.getWriter()).rejects.toThrowErrorMatchingInlineSnapshot(
`"There has been a catastrophic error trying to install namespace level resources for the following registration context: observability.apm. This may have been due to a non-additive change to the mappings, removal and type changes are not permitted. Full error: Error: bad resource installation"`
);
expect(mockLogger.error).toHaveBeenNthCalledWith(
expect(logger.error).toHaveBeenNthCalledWith(
1,
new RuleDataWriterInitializationError(
'namespace',
'observability.apm',
new Error('bad resource installation')
)
);
expect(mockLogger.error).toHaveBeenNthCalledWith(
expect(logger.error).toHaveBeenNthCalledWith(
2,
`The writer for the Rule Data Client for the observability.apm registration context was not initialized properly, bulk() cannot continue, and writing will be disabled.`
);
Expand All @@ -260,7 +270,7 @@ describe('RuleDataClient', () => {
await expect(() => ruleDataClient.getWriter()).rejects.toThrowErrorMatchingInlineSnapshot(
`"Rule registry writing is disabled due to an error during Rule Data Client initialization."`
);
expect(mockLogger.debug).toHaveBeenCalledWith(
expect(logger.debug).toHaveBeenCalledWith(
`Writing is disabled, bulk() will not write any data.`
);
});
Expand Down Expand Up @@ -297,7 +307,7 @@ describe('RuleDataClient', () => {
await delay();

expect(await writer.bulk({})).toEqual(undefined);
expect(mockLogger.debug).toHaveBeenCalledWith(
expect(logger.debug).toHaveBeenCalledWith(
`Writing is disabled, bulk() will not write any data.`
);
});
Expand All @@ -317,11 +327,16 @@ describe('RuleDataClient', () => {
await expect(() => writer.bulk({})).rejects.toThrowErrorMatchingInlineSnapshot(
`"something went wrong!"`
);
expect(mockLogger.error).toHaveBeenNthCalledWith(1, error);
expect(logger.error).toHaveBeenNthCalledWith(1, error);
expect(ruleDataClient.isWriteEnabled()).toBe(true);
});

test('waits until cluster client is ready before calling bulk', async () => {
scopedClusterClient.bulk.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(
{}
) as unknown as estypes.BulkResponse
);
const ruleDataClient = new RuleDataClient(
getRuleDataClientOptions({
waitUntilReadyForWriting: new Promise((resolve) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,17 @@ export class RuleDataClient implements IRuleDataClient {
return {
search: async (request) => {
const clusterClient = await waitUntilReady();
let body;
ymao1 marked this conversation as resolved.
Show resolved Hide resolved

const body = await clusterClient.search({
...request,
index: indexPattern,
});
try {
body = await clusterClient.search({
...request,
index: indexPattern,
});
} catch (err) {
this.options.logger.error(`Error performing search in RuleDataClient - ${err.message}`);
throw err;
}

return body as any;
ymao1 marked this conversation as resolved.
Show resolved Hide resolved
},
Expand All @@ -136,6 +142,9 @@ export class RuleDataClient implements IRuleDataClient {
title: indexPattern,
};
}
this.options.logger.error(
`Error fetching index patterns in RuleDataClient - ${err.message}`
);
throw err;
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,25 +95,32 @@ export class ResourceInstaller {
*/
public async installCommonResources(): Promise<void> {
await this.installWithTimeout('common resources shared between all indices', async () => {
const { getResourceName } = this.options;

// We can install them in parallel
await Promise.all([
this.createOrUpdateLifecyclePolicy({
name: getResourceName(DEFAULT_ILM_POLICY_ID),
body: defaultLifecyclePolicy,
}),

this.createOrUpdateComponentTemplate({
name: getResourceName(TECHNICAL_COMPONENT_TEMPLATE_NAME),
body: technicalComponentTemplate,
}),

this.createOrUpdateComponentTemplate({
name: getResourceName(ECS_COMPONENT_TEMPLATE_NAME),
body: ecsComponentTemplate,
}),
]);
const { getResourceName, logger } = this.options;

try {
// We can install them in parallel
await Promise.all([
this.createOrUpdateLifecyclePolicy({
name: getResourceName(DEFAULT_ILM_POLICY_ID),
body: defaultLifecyclePolicy,
}),

this.createOrUpdateComponentTemplate({
name: getResourceName(TECHNICAL_COMPONENT_TEMPLATE_NAME),
body: technicalComponentTemplate,
}),

this.createOrUpdateComponentTemplate({
name: getResourceName(ECS_COMPONENT_TEMPLATE_NAME),
body: ecsComponentTemplate,
}),
]);
} catch (err) {
logger.error(
`Error installing common resources in RuleRegistry ResourceInstaller - ${err.message}`
);
throw err;
}
});
}

Expand Down Expand Up @@ -375,6 +382,9 @@ export class ResourceInstaller {
},
});
} catch (err) {
logger.error(
`Error creating index ${initialIndexName} as the write index for alias ${primaryNamespacedAlias} in RuleRegistry ResourceInstaller: ${err.message}`
);
// If the index already exists and it's the write index for the alias,
// something else created it so suppress the error. If it's not the write
// index, that's bad, throw an error.
Expand Down Expand Up @@ -460,6 +470,9 @@ export class ResourceInstaller {
return createConcreteIndexInfo({});
}

logger.error(
`Error fetching concrete indices for ${indexPatternForBackingIndices} in RuleRegistry ResourceInstaller - ${err.message}`
);
// A non-404 error is a real problem so we re-throw.
throw err;
}
Expand Down