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.

Sign up for GitHub

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

[RAC][Rule Registry] Improve RuleDataService API and index bootstrapping implementation #108115

Merged
merged 13 commits into from
Aug 15, 2021
Merged

[RAC][Rule Registry] Improve RuleDataService API and index bootstrapping implementation #108115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ce85666
Old consolidation draft (TEMP, TO BE REMOVED)
banderror Aug 3, 2021
bac3d92
Improve the API of RuleDataService: draft of the new API
banderror Aug 9, 2021
7f53c4c
Improve the API of RuleDataService: clean up
banderror Aug 10, 2021
e83207a
Improve the API of RuleDataService: migrate plugins to the new API
banderror Aug 10, 2021
591dd18
Add TODO comments
banderror Aug 10, 2021
a061a4e
Index bootstrapping: create index template per namespace
banderror Aug 10, 2021
461db8d
Index bootstrapping: add secondary alias (.siem-signals) to security.…
banderror Aug 10, 2021
6ccccbb
Improve the API of RuleDataClient
banderror Aug 11, 2021
dccd175
Fix: const enums are not supported
banderror Aug 11, 2021
4ecf2e3
Remove EventLogService implementation
banderror Aug 11, 2021
926cada
Clean up
banderror Aug 11, 2021
826276b
Address code review feedback
banderror Aug 12, 2021
077ca24
Swap right and left, wrap initializeService result
marshallmain Aug 13, 2021
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
4 changes: 2 additions & 2 deletions x-pack/plugins/apm/server/lib/alerts/register_apm_alerts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { Observable } from 'rxjs';
import { Logger } from 'kibana/server';
import { PluginSetupContract as AlertingPluginSetupContract } from '../../../../alerting/server';
import { RuleDataClient } from '../../../../rule_registry/server';
import { IRuleDataClient } from '../../../../rule_registry/server';
import { registerTransactionDurationAlertType } from './register_transaction_duration_alert_type';
import { registerTransactionDurationAnomalyAlertType } from './register_transaction_duration_anomaly_alert_type';
import { registerErrorCountAlertType } from './register_error_count_alert_type';
Expand All @@ -17,7 +17,7 @@ import { MlPluginSetup } from '../../../../ml/server';
import { registerTransactionErrorRateAlertType } from './register_transaction_error_rate_alert_type';

export interface RegisterRuleDependencies {
ruleDataClient: RuleDataClient;
ruleDataClient: IRuleDataClient;
ml?: MlPluginSetup;
alerting: AlertingPluginSetupContract;
config$: Observable<APMConfig>;
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/apm/server/lib/alerts/test_utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { Logger } from 'kibana/server';
import { of } from 'rxjs';
import { elasticsearchServiceMock } from 'src/core/server/mocks';
import type { RuleDataClient } from '../../../../../rule_registry/server';
import type { IRuleDataClient } from '../../../../../rule_registry/server';
import { PluginSetupContract as AlertingPluginSetupContract } from '../../../../../alerting/server';
import { APMConfig, APM_SERVER_FEATURE_ID } from '../../..';

Expand Down Expand Up @@ -63,7 +63,8 @@ export const createRuleTypeMocks = () => {
};
},
isWriteEnabled: jest.fn(() => true),
} as unknown) as RuleDataClient,
indexName: '.alerts-observability.apm.alerts',
} as unknown) as IRuleDataClient,
},
services,
scheduleActions,
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/apm/server/lib/services/get_service_alerts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import type { EVENT_KIND as EVENT_KIND_TYPED } from '@kbn/rule-data-utils';
// @ts-expect-error
import { EVENT_KIND as EVENT_KIND_NON_TYPED } from '@kbn/rule-data-utils/target_node/technical_field_names';
import { RuleDataClient } from '../../../../rule_registry/server';
import { IRuleDataClient } from '../../../../rule_registry/server';
import {
SERVICE_NAME,
TRANSACTION_TYPE,
Expand All @@ -26,7 +26,7 @@ export async function getServiceAlerts({
environment,
transactionType,
}: {
ruleDataClient: RuleDataClient;
ruleDataClient: IRuleDataClient;
start: number;
end: number;
serviceName: string;
Expand Down
100 changes: 32 additions & 68 deletions x-pack/plugins/apm/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ import {
Plugin,
PluginInitializerContext,
} from 'src/core/server';
import { isEmpty, mapValues, once } from 'lodash';
import { isEmpty, mapValues } from 'lodash';
import { SavedObjectsClient } from '../../../../src/core/server';
import { TECHNICAL_COMPONENT_TEMPLATE_NAME } from '../../rule_registry/common/assets';
import { mappingFromFieldMap } from '../../rule_registry/common/mapping_from_field_map';
import { Dataset } from '../../rule_registry/server';
import { APMConfig, APMXPackConfig, APM_SERVER_FEATURE_ID } from '.';
import { mergeConfigs } from './index';
import { UI_SETTINGS } from '../../../../src/plugins/data/common';
Expand Down Expand Up @@ -106,79 +106,43 @@ export class APMPlugin

registerFeaturesUsage({ licensingPlugin: plugins.licensing });

const { ruleDataService } = plugins.ruleRegistry;
const getCoreStart = () =>
core.getStartServices().then(([coreStart]) => coreStart);

const alertsIndexPattern = ruleDataService.getFullAssetName(
'observability-apm*'
);

const initializeRuleDataTemplates = once(async () => {
const componentTemplateName = ruleDataService.getFullAssetName(
'apm-mappings'
);

if (!ruleDataService.isWriteEnabled()) {
return;
}

await ruleDataService.createOrUpdateComponentTemplate({
name: componentTemplateName,
body: {
template: {
settings: {
number_of_shards: 1,
},
mappings: mappingFromFieldMap(
{
[SERVICE_NAME]: {
type: 'keyword',
},
[SERVICE_ENVIRONMENT]: {
type: 'keyword',
},
[TRANSACTION_TYPE]: {
type: 'keyword',
},
[PROCESSOR_EVENT]: {
type: 'keyword',
},
const { ruleDataService } = plugins.ruleRegistry;
const ruleDataClient = ruleDataService.initializeIndex({
Copy link
Member

Choose a reason for hiding this comment

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

does this also create the index?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it doesn't, in that case maybe another name is in order? Something like installResources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgieselaar yes, it starts index bootstrapping right away inside the call in the background

Copy link
Contributor Author

Choose a reason for hiding this comment

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

installIndexResources or bootstrapIndex sounds good to me, more descriptive names. I can rename it in a follow-up PR 👍

Copy link
Member

Choose a reason for hiding this comment

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

first one sgtm 👍

feature: APM_SERVER_FEATURE_ID,
registrationContext: 'observability.apm',
dataset: Dataset.alerts,
componentTemplateRefs: [],
componentTemplates: [
{
name: 'mappings',
version: 0,
mappings: mappingFromFieldMap(
{
[SERVICE_NAME]: {
type: 'keyword',
},
'strict'
),
},
},
});

await ruleDataService.createOrUpdateIndexTemplate({
name: ruleDataService.getFullAssetName('apm-index-template'),
body: {
index_patterns: [alertsIndexPattern],
composed_of: [
ruleDataService.getFullAssetName(TECHNICAL_COMPONENT_TEMPLATE_NAME),
componentTemplateName,
],
[SERVICE_ENVIRONMENT]: {
type: 'keyword',
},
[TRANSACTION_TYPE]: {
type: 'keyword',
},
[PROCESSOR_EVENT]: {
type: 'keyword',
},
},
'strict'
),
},
});
await ruleDataService.updateIndexMappingsMatchingPattern(
alertsIndexPattern
);
],
indexTemplate: {
version: 0,
},
});

// initialize eagerly
const initializeRuleDataTemplatesPromise = initializeRuleDataTemplates().catch(
(err) => {
this.logger!.error(err);
}
);

const ruleDataClient = ruleDataService.getRuleDataClient(
APM_SERVER_FEATURE_ID,
ruleDataService.getFullAssetName('observability-apm'),
() => initializeRuleDataTemplatesPromise
);

const resourcePlugins = mapValues(plugins, (value, key) => {
return {
setup: value,
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/apm/server/routes/typings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
KibanaRequest,
CoreStart,
} from 'src/core/server';
import { RuleDataClient } from '../../../rule_registry/server';
import { IRuleDataClient } from '../../../rule_registry/server';
import { AlertingApiRequestHandlerContext } from '../../../alerting/server';
import type { RacApiRequestHandlerContext } from '../../../rule_registry/server';
import { LicensingApiRequestHandlerContext } from '../../../licensing/server';
Expand Down Expand Up @@ -72,6 +72,6 @@ export interface APMRouteHandlerResources {
start: () => Promise<Required<APMPluginDependencies>[key]['start']>;
};
};
ruleDataClient: RuleDataClient;
ruleDataClient: IRuleDataClient;
telemetryUsageCounter?: TelemetryUsageCounter;
}
63 changes: 15 additions & 48 deletions x-pack/plugins/infra/server/services/rules/rule_data_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@
* 2.0.
*/

import { once } from 'lodash';
import { CoreSetup, Logger } from 'src/core/server';
import { TECHNICAL_COMPONENT_TEMPLATE_NAME } from '../../../../rule_registry/common/assets';
import { RuleRegistryPluginSetupContract } from '../../../../rule_registry/server';
import { Dataset, RuleRegistryPluginSetupContract } from '../../../../rule_registry/server';
import type { InfraFeatureId } from '../../../common/constants';
import { RuleRegistrationContext, RulesServiceStartDeps } from './types';

Expand All @@ -25,51 +23,20 @@ export const createRuleDataClient = ({
logger: Logger;
ruleDataService: RuleRegistryPluginSetupContract['ruleDataService'];
}) => {
const initializeRuleDataTemplates = once(async () => {
const componentTemplateName = ruleDataService.getFullAssetName(
`${registrationContext}-mappings`
);

const indexNamePattern = ruleDataService.getFullAssetName(`${registrationContext}*`);

if (!ruleDataService.isWriteEnabled()) {
return;
}

await ruleDataService.createOrUpdateComponentTemplate({
name: componentTemplateName,
body: {
template: {
settings: {
number_of_shards: 1,
},
mappings: {},
},
},
});

await ruleDataService.createOrUpdateIndexTemplate({
name: ruleDataService.getFullAssetName(registrationContext),
body: {
index_patterns: [indexNamePattern],
composed_of: [
ruleDataService.getFullAssetName(TECHNICAL_COMPONENT_TEMPLATE_NAME),
componentTemplateName,
],
return ruleDataService.initializeIndex({
feature: ownerFeatureId,
registrationContext,
dataset: Dataset.alerts,
componentTemplateRefs: [],
componentTemplates: [
{
name: 'mappings',
version: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proper versioning according to the recent discussions will be implemented in a follow-up PR.

mappings: {},
},
});

await ruleDataService.updateIndexMappingsMatchingPattern(indexNamePattern);
});

// initialize eagerly
const initializeRuleDataTemplatesPromise = initializeRuleDataTemplates().catch((err) => {
logger.error(err);
],
indexTemplate: {
version: 0,
},
});

return ruleDataService.getRuleDataClient(
ownerFeatureId,
ruleDataService.getFullAssetName(registrationContext),
() => initializeRuleDataTemplatesPromise
);
};
4 changes: 2 additions & 2 deletions x-pack/plugins/infra/server/services/rules/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { PluginSetupContract as AlertingPluginSetup } from '../../../../alerting/server';
import {
createLifecycleExecutor,
RuleDataClient,
IRuleDataClient,
RuleRegistryPluginSetupContract,
} from '../../../../rule_registry/server';

Expand All @@ -24,7 +24,7 @@ export interface RulesServiceStartDeps {}

export interface RulesServiceSetup {
createLifecycleRuleExecutor: LifecycleRuleExecutorCreator;
ruleDataClient: RuleDataClient;
ruleDataClient: IRuleDataClient;
}

// eslint-disable-next-line @typescript-eslint/no-empty-interface
Expand Down
18 changes: 12 additions & 6 deletions x-pack/plugins/observability/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
ScopedAnnotationsClientFactory,
AnnotationsAPI,
} from './lib/annotations/bootstrap_annotations';
import type { RuleRegistryPluginSetupContract } from '../../rule_registry/server';
import { Dataset, RuleRegistryPluginSetupContract } from '../../rule_registry/server';
import { PluginSetupContract as FeaturesSetup } from '../../features/server';
import { uiSettings } from './ui_settings';
import { registerRoutes } from './routes/register_routes';
Expand Down Expand Up @@ -100,11 +100,17 @@ export class ObservabilityPlugin implements Plugin<ObservabilityPluginSetup> {

const start = () => core.getStartServices().then(([coreStart]) => coreStart);

const ruleDataClient = plugins.ruleRegistry.ruleDataService.getRuleDataClient(
'observability',
plugins.ruleRegistry.ruleDataService.getFullAssetName(),
() => Promise.resolve()
);
const { ruleDataService } = plugins.ruleRegistry;
const ruleDataClient = ruleDataService.initializeIndex({
feature: 'observability',
registrationContext: 'observability',
dataset: Dataset.alerts,
componentTemplateRefs: [],
componentTemplates: [],
indexTemplate: {
version: 0,
},
});
Comment on lines +103 to +113
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replaces the previous implementation:

    const ruleDataClient = plugins.ruleRegistry.ruleDataService.getRuleDataClient(
      'observability',
      plugins.ruleRegistry.ruleDataService.getFullAssetName(),
      () => Promise.resolve()
    );

Seems like it's a stub. Should I keep it? Why it's needed?

Copy link
Member

Choose a reason for hiding this comment

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

The observability plugin apparently needs a ruleDataClient for use in some route handlers without intending to write any alerts itself. Does this initializeIndex create any assets which the previous implementation didn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this initializeIndex create any assets which the previous implementation didn't?

It will see if it should bootstrap any resources for .alerts-observability. Specifically:

  • ILM policy if it's provided. In this case, it's not.
  • Component templates if they are provided. In this case, they're not.

And then it will update mappings of all concrete indices matching .alerts-observability-*. This pattern shouldn't match any indices, I guess, because you're saying this ruleDataClient is not used for writing alerts? Other indices containing alerts are named like .alerts-observability.apm-* etc.

Is this ruleDataClient used for reading all the observability alerts - from .alerts-observability.apm-*, .alerts-observability.logs-* and other indices?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems for this use case it would be useful to expose some kind of getRuleDataReader API in RuleDataPluginService that skips all the index management steps and just returns an IRuleDataReader. If I understand correctly the registrationContext would be observability.*.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a ticket for that: #111173


registerRoutes({
core: {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/observability/server/routes/register_routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import { CoreSetup, CoreStart, Logger, RouteRegistrar } from 'kibana/server';
import Boom from '@hapi/boom';
import { RequestAbortedError } from '@elastic/elasticsearch/lib/errors';
import { RuleDataClient } from '../../../rule_registry/server';
import { IRuleDataClient } from '../../../rule_registry/server';
import { ObservabilityRequestHandlerContext } from '../types';
import { AbstractObservabilityServerRouteRepository } from './types';

Expand All @@ -29,7 +29,7 @@ export function registerRoutes({
};
repository: AbstractObservabilityServerRouteRepository;
logger: Logger;
ruleDataClient: RuleDataClient;
ruleDataClient: IRuleDataClient;
}) {
const routes = repository.getRoutes();

Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/observability/server/routes/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type {
ServerRouteRepository,
} from '@kbn/server-route-repository';
import { CoreSetup, CoreStart, KibanaRequest, Logger } from 'kibana/server';
import { RuleDataClient } from '../../../rule_registry/server';
import { IRuleDataClient } from '../../../rule_registry/server';

import { ObservabilityServerRouteRepository } from './get_global_observability_server_route_repository';
import { ObservabilityRequestHandlerContext } from '../types';
Expand All @@ -24,7 +24,7 @@ export interface ObservabilityRouteHandlerResources {
start: () => Promise<CoreStart>;
setup: CoreSetup;
};
ruleDataClient: RuleDataClient;
ruleDataClient: IRuleDataClient;
request: KibanaRequest;
context: ObservabilityRequestHandlerContext;
logger: Logger;
Expand Down
4 changes: 0 additions & 4 deletions x-pack/plugins/rule_registry/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@

import { estypes } from '@elastic/elasticsearch';

export type PutIndexTemplateRequest = estypes.IndicesPutIndexTemplateRequest & {
body?: { composed_of?: string[] };
};

export interface ClusterPutComponentTemplateBody {
template: {
settings: {
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/rule_registry/kibana.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"requiredPlugins": [
"alerting",
"data",
"spaces",
"triggersActionsUi"
],
"optionalPlugins": ["security"],
Expand Down
14 changes: 0 additions & 14 deletions x-pack/plugins/rule_registry/server/event_log/elasticsearch/index.ts
View file
Open in desktop

This file was deleted.

Loading