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

[Cloud Security][Telemetry] add error handling incase an individual collector fails #165918

Merged
merged 9 commits into from
Sep 13, 2023
Merged

[Cloud Security][Telemetry] add error handling incase an individual collector fails #165918

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
21ba89e
add error handling incase an individual collector fails
Omolola-Akinleye Sep 6, 2023
e97c821
Merge branch 'main' into fix_telemetry_bug_usage_collector_failure
Omolola-Akinleye Sep 7, 2023
d3b8990
address pr comments
Omolola-Akinleye Sep 11, 2023
283bd81
Merge branch 'main' into fix_telemetry_bug_usage_collector_failure
Omolola-Akinleye Sep 11, 2023
421a4f3
Merge branch 'main' into fix_telemetry_bug_usage_collector_failure
Omolola-Akinleye Sep 11, 2023
ed90ea8
address nit comment
Omolola-Akinleye Sep 13, 2023
73531f6
Merge branch 'main' into fix_telemetry_bug_usage_collector_failure
Omolola-Akinleye Sep 13, 2023
b54b3a7
type issue and return error
Omolola-Akinleye Sep 13, 2023
08c0921
Merge branch 'fix_telemetry_bug_usage_collector_failure' of github.co…
Omolola-Akinleye Sep 13, 2023
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
2 changes: 1 addition & 1 deletion ...ns/cloud_security_posture/server/lib/telemetry/collectors/installation_stats_collector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const getEnabledInputStreamVars = (packagePolicy: PackagePolicy) => {
const getAccountTypeField = (
packagePolicy: PackagePolicy
): CloudSecurityInstallationStats['account_type'] => {
if (packagePolicy.vars?.posture.value !== 'cspm') return;
if (packagePolicy.vars?.posture?.value !== 'cspm') return;

const inputStreamVars = getEnabledInputStreamVars(packagePolicy);
const cloudProvider = packagePolicy.vars?.deployment?.value;
Expand Down
30 changes: 21 additions & 9 deletions x-pack/plugins/cloud_security_posture/server/lib/telemetry/collectors/register.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { CspServerPluginStart, CspServerPluginStartDeps } from '../../../types';
import { getIndicesStats } from './indices_stats_collector';
import { getResourcesStats } from './resources_stats_collector';
import { cspmUsageSchema } from './schema';
import { CspmUsage } from './types';
import { CspmUsage, type CloudSecurityUsageCollectorType } from './types';
import { getAccountsStats } from './accounts_stats_collector';
import { getRulesStats } from './rules_stats_collector';
import { getInstallationStats } from './installation_stats_collector';
Expand All @@ -35,14 +35,19 @@ export function registerCspmUsageCollector(
return true;
},
fetch: async (collectorFetchContext: CollectorFetchContext) => {
const [
indicesStats,
accountsStats,
resourcesStats,
rulesStats,
installationStats,
alertsStats,
] = await Promise.all([
const getPromiseValue = <T>(
Copy link
Contributor

@kfirpeled kfirpeled Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit you could improve it a bit by not tangling yourself with results[X] and working with it by using the following changing getPromiseValue to receive a promise and wait for it
So when forming the allSettled request, the naming of the request is right next to it.
And now getPromiseValue name also describes better its logic.
And maybe awaitPromiseSafe describes it even better. It is safe because it won't throw any errors

      const awaitPromiseSafe = async <T>(
        taskName: CloudSecurityUsageCollectorType,
        promise: Promise<T>
      ) => {
        try {
          const val = await promise;
          return val;
        } catch (error) {
          logger.error(`${taskName} task failed: ${error.message}`);
          logger.error(error.stack);
          return undefined;
        }
      };
      const esClient = collectorFetchContext.esClient;
      const soClient = collectorFetchContext.soClient;
      const [
        indices,
        // eslint-disable-next-line @typescript-eslint/naming-convention
        accounts_stats,
        // eslint-disable-next-line @typescript-eslint/naming-convention
        resources_stats,
        // eslint-disable-next-line @typescript-eslint/naming-convention
        rules_stats,
        // eslint-disable-next-line @typescript-eslint/naming-convention
        installation_stats,
        // eslint-disable-next-line @typescript-eslint/naming-convention
        alerts_stats,
      ] = await Promise.all([
        awaitPromiseSafe('Indices', getIndicesStats(esClient, soClient, coreServices, logger)),
        awaitPromiseSafe('Accounts', getAccountsStats(esClient, logger)),
        awaitPromiseSafe('Resources', getResourcesStats(esClient, logger)),
        awaitPromiseSafe('Rules', getRulesStats(esClient, logger)),
        awaitPromiseSafe(
          'Installation',
          getInstallationStats(esClient, soClient, coreServices, logger)
        ),
        awaitPromiseSafe('Alerts', getAlertsStats(esClient, logger)),
      ]);

      return {
        indices,
        accounts_stats,
        resources_stats,
        rules_stats,
        installation_stats,
        alerts_stats,
      };

Copy link
Contributor

Choose a reason for hiding this comment

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

and we can replace allSettled with all, because we wrap it with a handler that keeps it safe

taskName: CloudSecurityUsageCollectorType,
result: PromiseSettledResult<T>
) => {
if (result.status === 'fulfilled') {
return result.value;
} else {
logger.error(`${taskName} task failed: ${result.reason}`);

return undefined;
}
};
const results = await Promise.allSettled([
getIndicesStats(
collectorFetchContext.esClient,
collectorFetchContext.soClient,
Expand All @@ -61,6 +66,13 @@ export function registerCspmUsageCollector(
getAlertsStats(collectorFetchContext.esClient, logger),
]);

const indicesStats = getPromiseValue('Indices', results[0]);
const accountsStats = getPromiseValue('Accounts', results[1]);
const resourcesStats = getPromiseValue('Resources', results[2]);
const rulesStats = getPromiseValue('Rules', results[3]);
const installationStats = getPromiseValue('Installation', results[4]);
const alertsStats = getPromiseValue('Alerts', results[5]);

return {
indices: indicesStats,
accounts_stats: accountsStats,
Expand Down
20 changes: 14 additions & 6 deletions x-pack/plugins/cloud_security_posture/server/lib/telemetry/collectors/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,21 @@

import { CspStatusCode } from '../../../../common/types';

export type CloudSecurityUsageCollectorType =
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

| 'Indices'
| 'Accounts'
| 'Resources'
| 'Rules'
| 'Installation'
| 'Alerts';

export interface CspmUsage {
indices: CspmIndicesStats;
resources_stats: CspmResourcesStats[];
accounts_stats: CspmAccountsStats[];
rules_stats: CspmRulesStats[];
installation_stats: CloudSecurityInstallationStats[];
alerts_stats: CloudSecurityAlertsStats[];
indices: CspmIndicesStats | undefined;
resources_stats: CspmResourcesStats[] | undefined;
accounts_stats: CspmAccountsStats[] | undefined;
rules_stats: CspmRulesStats[] | undefined;
installation_stats: CloudSecurityInstallationStats[] | undefined;
alerts_stats: CloudSecurityAlertsStats[] | undefined;
}

export interface PackageSetupStatus {
Expand Down