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

chore(slo): cleanup error log message #205334

Merged
merged 3 commits into from
Jan 2, 2025
Merged

chore(slo): cleanup error log message #205334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76b632c
Audit SLO logged errors
kdelemme Dec 30, 2024
c87726d
Include original error in log message
kdelemme Dec 31, 2024
5d395f1
Merge branch 'main' into chore/slo-logger-errors
kdelemme Jan 2, 2025
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/solutions/observability/plugins/slo/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,8 @@ export class SLOPlugin
const sloInstaller = new DefaultSLOInstaller(sloResourceInstaller, this.logger);
await sloInstaller.install();
})
.catch((error) => {
this.logger.error(`Failed to install the default SLOs: ${error}`);
.catch(() => {
// noop - error already logged from the installer
});

this.sloOrphanCleanupTask = new SloOrphanSummaryCleanupTask(
Expand Down
4 changes: 2 additions & 2 deletions x-pack/solutions/observability/plugins/slo/server/services/create_slo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,14 @@ export class CreateSLO {
]);
} catch (err) {
this.logger.error(
`Cannot install the SLO [id: ${slo.id}, revision: ${slo.revision}]. Rolling back.`
`Cannot create the SLO [id: ${slo.id}, revision: ${slo.revision}]. Rolling back. ${err}`
Copy link
Member

Choose a reason for hiding this comment

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

Isn't err an object? How would it be logged in this case?
I vaguely remember there was a consideration while logging errors to ensure we don't log sensitive information, do you remember what the issue was and whether it is applicable in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, it would be just formatted as error string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's an object, it will be serialized using its toString() method, which by default includes the name, message and stack trace I believe.

I vaguely remember there was a consideration while logging errors to ensure we don't log sensitive information, do you remember what the issue was and whether it is applicable in this PR?

This was mostly about logging request/response details. In this case, the error message should not include any sensitive information.

);

await asyncForEach(rollbackOperations.reverse(), async (operation) => {
try {
await operation();
} catch (rollbackErr) {
this.logger.error('Rollback operation failed', rollbackErr);
this.logger.error(`Rollback operation failed. ${rollbackErr}`);
}
});

Expand Down
2 changes: 1 addition & 1 deletion x-pack/solutions/observability/plugins/slo/server/services/reset_slo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export class ResetSLO {
);
} catch (err) {
this.logger.error(
`Cannot reset the SLO [id: ${slo.id}, revision: ${slo.revision}]. Rolling back.`
`Cannot reset the SLO [id: ${slo.id}, revision: ${slo.revision}]. Rolling back. ${err}`
);

await this.summaryTransformManager.stop(summaryTransformId);
Expand Down
2 changes: 1 addition & 1 deletion x-pack/solutions/observability/plugins/slo/server/services/resource_installer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export class DefaultResourceInstaller implements ResourceInstaller {
await this.createIndex(SLO_SUMMARY_DESTINATION_INDEX_NAME);
await this.createIndex(SLO_SUMMARY_TEMP_INDEX_NAME);
} catch (err) {
this.logger.error(`Error installing resources shared for SLO: ${err.message}`);
this.logger.error(`Error while installing SLO shared resources: ${err}`);
throw err;
}
}
Expand Down
2 changes: 1 addition & 1 deletion x-pack/solutions/observability/plugins/slo/server/services/slo_installer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export class DefaultSLOInstaller implements SLOInstaller {

await this.sloResourceInstaller.ensureCommonResourcesInstalled();
} catch (error) {
this.logger.error('Failed to install SLO common resources');
this.logger.error(`Failed to install SLO common resources: ${error}`);
} finally {
this.isInstalling = false;
clearTimeout(installTimeout);
Expand Down
2 changes: 1 addition & 1 deletion x-pack/solutions/observability/plugins/slo/server/services/summary_search_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export class DefaultSummarySearchClient implements SummarySearchClient {
}),
};
} catch (err) {
this.logger.error(new Error(`Summary search query error, ${err.message}`, { cause: err }));
this.logger.error(`Error while searching SLO summary documents. ${err}`);
return { total: 0, perPage: pagination.perPage, page: pagination.page, results: [] };
}
}
Expand Down
10 changes: 5 additions & 5 deletions x-pack/solutions/observability/plugins/slo/server/services/summay_transform_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export class DefaultSummaryTransformManager implements TransformManager {
}
);
} catch (err) {
this.logger.error(`Cannot create summary transform for SLO [${slo.id}]`);
this.logger.error(`Cannot create summary transform for SLO [${slo.id}]. ${err}`);
if (err.meta?.body?.error?.type === 'security_exception') {
throw new SecurityException(err.meta.body.error.reason);
}
Expand All @@ -57,7 +57,7 @@ export class DefaultSummaryTransformManager implements TransformManager {
{ logger: this.logger }
);
} catch (err) {
this.logger.error(`Cannot preview SLO summary transform [${transformId}]`);
this.logger.error(`Cannot preview SLO summary transform [${transformId}]. ${err}`);
throw err;
}
}
Expand All @@ -75,7 +75,7 @@ export class DefaultSummaryTransformManager implements TransformManager {
}
);
} catch (err) {
this.logger.error(`Cannot start SLO summary transform [${transformId}]`);
this.logger.error(`Cannot start SLO summary transform [${transformId}]. ${err}`);
throw err;
}
}
Expand All @@ -91,7 +91,7 @@ export class DefaultSummaryTransformManager implements TransformManager {
{ logger: this.logger }
);
} catch (err) {
this.logger.error(`Cannot stop SLO summary transform [${transformId}]`);
this.logger.error(`Cannot stop SLO summary transform [${transformId}]. ${err}`);
throw err;
}
}
Expand All @@ -107,7 +107,7 @@ export class DefaultSummaryTransformManager implements TransformManager {
{ logger: this.logger }
);
} catch (err) {
this.logger.error(`Cannot delete SLO summary transform [${transformId}]`);
this.logger.error(`Cannot delete SLO summary transform [${transformId}]. ${err}`);
throw err;
}
}
Expand Down
10 changes: 6 additions & 4 deletions .../solutions/observability/plugins/slo/server/services/tasks/orphan_summary_cleanup_task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export class SloOrphanSummaryCleanupTask {
},

cancel: async () => {
this.abortController.abort('[SLO] Definitions clean up Task timed out');
this.abortController.abort('orphan-slo-summary-cleanup task timed out');
},
};
},
Expand Down Expand Up @@ -101,7 +101,7 @@ export class SloOrphanSummaryCleanupTask {

if (sloSummaryIdsToDelete.length > 0) {
this.logger.info(
`[SLO] Deleting ${sloSummaryIdsToDelete.length} SLO Summaries from the summary index`
`[SLO] Deleting ${sloSummaryIdsToDelete.length} SLO Summary documents from the summary index`
);

await this.esClient.deleteByQuery({
Expand All @@ -124,7 +124,7 @@ export class SloOrphanSummaryCleanupTask {
searchAfter?: AggregationsCompositeAggregateKey;
sloSummaryIds: Array<{ id: string; revision: number }>;
}> => {
this.logger.debug(`[SLO] Fetching SLO Summaries ids after ${searchAfter}`);
this.logger.debug(`[TASK] Fetching SLO Summary ids after ${searchAfter}`);
if (!this.esClient) {
return {
searchAfter: undefined,
Expand Down Expand Up @@ -227,7 +227,9 @@ export class SloOrphanSummaryCleanupTask {
this.esClient = esClient;

if (!taskManager) {
this.logger.info('[SLO] Missing required service during startup, skipping task.');
this.logger.info(
'Missing required service during startup, skipping orphan-slo-summary-cleanup task.'
);
return;
}

Expand Down
2 changes: 1 addition & 1 deletion x-pack/solutions/observability/plugins/slo/server/services/transform_generators/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function parseStringFilters(filters: string, logger: Logger) {
try {
return JSON.parse(filters);
} catch (e) {
logger.error(`Failed to parse filters: ${e.message}`);
logger.info(`Failed to parse filters: ${e}`);
}

return {};
Expand Down
15 changes: 8 additions & 7 deletions x-pack/solutions/observability/plugins/slo/server/services/transform_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ export class DefaultTransformManager implements TransformManager {
}
);
} catch (err) {
this.logger.error(`Cannot create SLO transform for indicator type [${slo.indicator.type}]`);
this.logger.error(
`Cannot create SLO transform for indicator type [${slo.indicator.type}]. ${err}`
);
if (err.meta?.body?.error?.type === 'security_exception') {
throw new SecurityException(err.meta.body.error.reason);
}
Expand Down Expand Up @@ -77,7 +79,7 @@ export class DefaultTransformManager implements TransformManager {
{ logger: this.logger }
);
} catch (err) {
this.logger.error(`Cannot preview SLO transform [${transformId}]`);
this.logger.error(`Cannot preview SLO transform [${transformId}]. ${err}`);
throw err;
}
}
Expand All @@ -94,7 +96,7 @@ export class DefaultTransformManager implements TransformManager {
);
await this.scheduleNowTransform(transformId);
} catch (err) {
this.logger.error(`Cannot start SLO transform [${transformId}]`);
this.logger.error(`Cannot start SLO transform [${transformId}]. ${err}`);
throw err;
}
}
Expand All @@ -110,7 +112,7 @@ export class DefaultTransformManager implements TransformManager {
{ logger: this.logger }
);
} catch (err) {
this.logger.error(`Cannot stop SLO transform [${transformId}]`);
this.logger.error(`Cannot stop SLO transform [${transformId}]. ${err}`);
throw err;
}
}
Expand All @@ -126,7 +128,7 @@ export class DefaultTransformManager implements TransformManager {
{ logger: this.logger }
);
} catch (err) {
this.logger.error(`Cannot delete SLO transform [${transformId}]`);
this.logger.error(`Cannot delete SLO transform [${transformId}]. ${err}`);
throw err;
}
}
Expand All @@ -138,8 +140,7 @@ export class DefaultTransformManager implements TransformManager {
this.logger.debug(`SLO transform [${transformId}] scheduled now successfully`);
})
.catch((e) => {
this.logger.error(`Cannot schedule now SLO transform [${transformId}]`);
this.logger.error(e);
this.logger.error(`Cannot schedule now SLO transform [${transformId}]. ${e}`);
});
}
}
8 changes: 4 additions & 4 deletions x-pack/solutions/observability/plugins/slo/server/services/update_slo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,14 @@ export class UpdateSLO {
);
} catch (err) {
this.logger.error(
`Cannot update the SLO summary pipeline [id: ${updatedSlo.id}, revision: ${updatedSlo.revision}].`
`Cannot update the SLO summary pipeline [id: ${updatedSlo.id}, revision: ${updatedSlo.revision}]. ${err}`
);

await asyncForEach(rollbackOperations.reverse(), async (operation) => {
try {
await operation();
} catch (rollbackErr) {
this.logger.error('Rollback operation failed', rollbackErr);
this.logger.error(`Rollback operation failed. ${rollbackErr}`);
}
});

Expand Down Expand Up @@ -182,14 +182,14 @@ export class UpdateSLO {
);
} catch (err) {
this.logger.error(
`Cannot update the SLO [id: ${updatedSlo.id}, revision: ${updatedSlo.revision}]. Rolling back.`
`Cannot update the SLO [id: ${updatedSlo.id}, revision: ${updatedSlo.revision}]. Rolling back. ${err}`
);

await asyncForEach(rollbackOperations.reverse(), async (operation) => {
try {
await operation();
} catch (rollbackErr) {
this.logger.error('Rollback operation failed', rollbackErr);
this.logger.error(`Rollback operation failed. ${rollbackErr}`);
}
});

Expand Down
Loading