Skip to content

Commit

Permalink
Address code review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
banderror committed Aug 12, 2021
1 parent 74b90d4 commit 4954403
Show file tree
Hide file tree
Showing 40 changed files with 552 additions and 405 deletions.
4 changes: 2 additions & 2 deletions x-pack/plugins/apm/server/lib/alerts/register_apm_alerts.ts
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
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
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
3 changes: 0 additions & 3 deletions x-pack/plugins/apm/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,6 @@ export class APMPlugin
{
name: 'mappings',
version: 0,
settings: {
number_of_shards: 1,
},
mappings: mappingFromFieldMap(
{
[SERVICE_NAME]: {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/apm/server/routes/typings.ts
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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ export const createRuleDataClient = ({
{
name: 'mappings',
version: 0,
settings: {
number_of_shards: 1,
},
mappings: {},
},
],
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/infra/server/services/rules/types.ts
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
4 changes: 2 additions & 2 deletions x-pack/plugins/observability/server/routes/register_routes.ts
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,84 +6,70 @@
*/

import { ResponseError } from '@elastic/elasticsearch/lib/errors';
import { ValidFeatureId } from '@kbn/rule-data-utils/target/alerts_as_data_rbac';

import { ElasticsearchClient } from 'kibana/server';
import { IndexPatternsFetcher } from '../../../../../src/plugins/data/server';

import { RuleDataWriteDisabledError } from '../rule_data_plugin_service/errors';
import { IndexNames } from '../rule_data_plugin_service/index_names';
import { IndexOptions } from '../rule_data_plugin_service/index_options';
import { IndexInfo } from '../rule_data_plugin_service/index_info';
import { ResourceInstaller } from '../rule_data_plugin_service/resource_installer';
import { IRuleDataClient, IRuleDataReader, IRuleDataWriter } from './types';

/**
* The purpose of the `feature` param is to force the user to update
* the data structure which contains the mapping of consumers to alerts
* as data indices. The idea is it is typed such that it forces the
* user to go to the code and modify it. At least until a better system
* is put in place or we move the alerts as data client out of rule registry.
*/
interface ConstructorOptions {
feature: ValidFeatureId;
indexNames: IndexNames;
indexOptions: IndexOptions;
indexInfo: IndexInfo;
resourceInstaller: ResourceInstaller;
isWriteEnabled: boolean;
waitUntilIndexIsReady: () => Promise<{ clusterClient: ElasticsearchClient }>;
waitUntilReadyForReading: Promise<ElasticsearchClient>;
waitUntilReadyForWriting: Promise<ElasticsearchClient>;
}

export class RuleDataClient implements IRuleDataClient {
constructor(private readonly options: ConstructorOptions) {}

public get indexName(): string {
return this.options.indexNames.baseName;
return this.options.indexInfo.baseName;
}

public isWriteEnabled(): boolean {
return this.options.isWriteEnabled;
}

public getReader(options: { namespace?: string } = {}): IRuleDataReader {
const { indexNames, waitUntilIndexIsReady } = this.options;

// Because namespace is user-defined in general, by default we want to
// ignore namespace when reading, and search over all the namespaces.
const namespace = options.namespace || '*';
const indexAliasOrPattern = indexNames.getPrimaryAlias(namespace);
const { indexInfo, waitUntilReadyForReading } = this.options;
const indexPattern = indexInfo.getPatternForReading(options.namespace);

return {
search: async (request) => {
const { clusterClient } = await waitUntilIndexIsReady();
const clusterClient = await waitUntilReadyForReading;

const { body } = (await clusterClient.search({
...request,
index: indexAliasOrPattern,
index: indexPattern,
})) as { body: any };

return body;
},

getDynamicIndexPattern: async () => {
const { clusterClient } = await waitUntilIndexIsReady();
const clusterClient = await waitUntilReadyForReading;
const indexPatternsFetcher = new IndexPatternsFetcher(clusterClient);

try {
const fields = await indexPatternsFetcher.getFieldsForWildcard({
pattern: indexAliasOrPattern,
pattern: indexPattern,
});

return {
fields,
timeFieldName: '@timestamp',
title: indexAliasOrPattern,
title: indexPattern,
};
} catch (err) {
if (err.output?.payload?.code === 'no_matching_indices') {
return {
fields: [],
timeFieldName: '@timestamp',
title: indexAliasOrPattern,
title: indexPattern,
};
}
throw err;
Expand All @@ -93,10 +79,10 @@ export class RuleDataClient implements IRuleDataClient {
}

public getWriter(options: { namespace?: string } = {}): IRuleDataWriter {
const { indexNames, indexOptions, resourceInstaller, waitUntilIndexIsReady } = this.options;
const { indexInfo, resourceInstaller, waitUntilReadyForWriting } = this.options;

const namespace = options.namespace || 'default';
const alias = indexNames.getPrimaryAlias(namespace);
const alias = indexInfo.getPrimaryAlias(namespace);
const isWriteEnabled = this.isWriteEnabled();

return {
Expand All @@ -105,7 +91,7 @@ export class RuleDataClient implements IRuleDataClient {
throw new RuleDataWriteDisabledError();
}

const { clusterClient } = await waitUntilIndexIsReady();
const clusterClient = await waitUntilReadyForWriting;

const requestWithDefaultParameters = {
...request,
Expand All @@ -125,7 +111,7 @@ export class RuleDataClient implements IRuleDataClient {
))
) {
return resourceInstaller
.createWriteTargetIfNeeded(indexOptions, indexNames, namespace)
.installNamespaceLevelResources(indexInfo, namespace)
.then(() => {
return clusterClient.bulk(requestWithDefaultParameters).then((retryResponse) => {
if (retryResponse.body.errors) {
Expand Down
Loading

0 comments on commit 4954403

Please sign in to comment.