[SLO] Bulk Purge SLI data#218287
Conversation
c428426 to
a88e2ba
Compare
tweaks and tests
f543477 to
4e4a247
Compare
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
| const bulkPurgeRollupSchema = t.intersection([ | ||
| t.partial({ | ||
| query: t.partial({ | ||
| force: t.string, | ||
| }), | ||
| }), | ||
| t.type({ | ||
| body: t.type({ | ||
| ids: t.array(t.string), | ||
| purgePolicy, | ||
| }), | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
Might be simpler to move force into the body as well
| const repository = new KibanaSavedObjectsSLORepository(soClient, logger); | ||
| const purgeRollupData = new PurgeRollupData(esClient, repository); | ||
|
|
||
| await purgeRollupData.execute(params); |
There was a problem hiding this comment.
params contains body and query, which are considered implementation details of the route handler. The PurgeRollupData service should not know anything about a body or a query (http specific)
Also, if we move force into the body, you can then use a Params type like:
type BulkPurgeRollupDataParams = t.TypeOf<typeof bulkPurgeRollupSchema.props.body>;
kdelemme
left a comment
There was a problem hiding this comment.
Just a few naming convention, imports and schema/type to fix. And use the slo list from the repository for the delete by query ids
| if (purgePolicy.purgeType === 'fixed_age') { | ||
| return `now-${purgePolicy.age.format()}`; | ||
| } else { | ||
| return purgePolicy.timestamp.toISOString(); | ||
| } |
There was a problem hiding this comment.
can you add an assertNever(purgeType);?
|
|
||
| private async validatePurgePolicy(params: PurgeRollupSchemaType) { | ||
| const { ids, purgePolicy } = params; | ||
| const slos = await this.repository.findAllByIds(ids); |
There was a problem hiding this comment.
I think we should fetch the slo list outside of the validation, and provide it to this function, and also use the sloList in the deleteByQuery: { 'slo.id': map(sloList, "id") }. so we don't delete slo that the user should not have access to.
| bool: { | ||
| filter: [ | ||
| { | ||
| terms: { 'slo.id': params.ids }, |
There was a problem hiding this comment.
So we don't delete SLOs that the user should not have access to.
| terms: { 'slo.id': params.ids }, | |
| terms: { 'slo.id': map(sloList, "id") }, |
| return moment(timestamp).isAfter( | ||
| moment(Date.now()).startOf(slo.timeWindow.duration.unit) | ||
| ); | ||
| } else { | ||
| return moment(timestamp).isAfter( | ||
| moment(Date.now()).subtract(slo.timeWindow.duration.asSeconds(), 's') | ||
| ); |
There was a problem hiding this comment.
nitpick: create moment(timestamp) once outside the some predicate, so we don't recreate one every time.
There was a problem hiding this comment.
very fair
| import { KibanaSavedObjectsSLORepository } from '../../services'; | ||
|
|
||
| export const purgeRollupDataRoute = createSloServerRoute({ | ||
| endpoint: 'POST /api/observability/slos/_purge_rollup 2023-10-31', |
There was a problem hiding this comment.
| endpoint: 'POST /api/observability/slos/_purge_rollup 2023-10-31', | |
| endpoint: 'POST /api/observability/slos/_bulk_purge_rollup 2023-10-31', |
| import { durationType } from './duration'; | ||
| import { indicatorSchema } from './indicators'; | ||
| import { timeWindowSchema } from './time_window'; | ||
| import { bulkPurgeRollupSchema } from '../rest_specs/routes/bulk_purge_rollup'; |
There was a problem hiding this comment.
don't import the rest_specs here.
| type PurgePolicyType = t.TypeOf<typeof purgePolicy>; | ||
| type PurgeRollupSchemaType = t.TypeOf<typeof bulkPurgeRollupSchema.props.body>; | ||
|
|
||
| export type { PurgePolicyType, PurgeRollupSchemaType }; |
There was a problem hiding this comment.
add export * from './bulk_purge_rollup'; into rest_specs/routes/index.ts
| }); | ||
|
|
||
| type PurgePolicyType = t.TypeOf<typeof purgePolicy>; | ||
| type PurgeRollupSchemaType = t.TypeOf<typeof bulkPurgeRollupSchema.props.body>; |
There was a problem hiding this comment.
Can you name the schema and type like the other schemas to be consistent, and introduce the response type as well e.g.
const bulkPurgeRollupParamsSchema = t.type({ ... })
const bulkPurgeRollupResponseSchema = t.type({ taskId: t.string });
type BulkPurgeRollupInput = t.OutputOf<typeof bulkPurgeRollupParamsSchema.props.body>;
type BulkPurgeRollupParams = t.TypeOf<typeof bulkPurgeRollupParamsSchema.props.body>;
type BulkPurgeRollupResponse = t.TypeOf<typeof bulkPurgeRollupResponseSchema.props.body>;
| import { PurgeRollupData } from '../../services/purge_rollup_data'; | ||
| import { KibanaSavedObjectsSLORepository } from '../../services'; | ||
|
|
||
| export const purgeRollupDataRoute = createSloServerRoute({ |
There was a problem hiding this comment.
can you name that bulkPurgeRollupRoute to be consistent with the types and url
|
|
||
| public async execute( | ||
| params: PurgeRollupSchemaType | ||
| ): Promise<{ taskId: DeleteByQueryResponse['task'] }> { |
There was a problem hiding this comment.
Use the BulkPurgeRollupResponse here
kdelemme
left a comment
There was a problem hiding this comment.
LGTM thanks for the changes
|
@baileycash-elastic Can you merge main in, and use the new getScopedClient(request) from the route handler? |
csr
left a comment
There was a problem hiding this comment.
x-pack/test/api_integration/deployment_agnostic/services/slo_api.ts purge endpoint change LGTM.
|
Starting backport for target branches: 8.17, 8.18, 8.19 https://github.com/elastic/kibana/actions/runs/14605402192 |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
|
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
|
Starting backport for target branches: 8.17, 8.18, 8.19 https://github.com/elastic/kibana/actions/runs/14605533214 |
|
@baileycash-elastic I've removed the backport:prev-major. we should only backport to 9.1 and 8.19 |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
## Summary Resolves elastic#210025 Introduces starter API for bulk purging of SLI data. **docs coming pending review** Users can submit a list of SLO ids for which they would like to purge rollup data, as well as a purge policy: ``` "purgePolicy": { "purgeType": "fixed_age", "age": "1w" } ``` ``` "purgePolicy": { "purgeType": "fixed_time", "age": "2025-04-01T00:00:00Z" } ``` --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 2df2e78) # Conflicts: # oas_docs/output/kibana.yaml # x-pack/solutions/observability/plugins/slo/docs/openapi/slo/bundled.json # x-pack/solutions/observability/plugins/slo/docs/openapi/slo/bundled.yaml # x-pack/solutions/observability/plugins/slo/docs/openapi/slo/entrypoint.yaml
|
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
# Backport This will backport the following commits from `main` to `8.19`: - [[SLO] Bulk Purge SLI data (#218287)](#218287) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Bailey Cash","email":"bailey.cash@elastic.co"},"sourceCommit":{"committedDate":"2025-04-22T21:58:31Z","message":"[SLO] Bulk Purge SLI data (#218287)\n\n## Summary\n\nResolves #210025 \n\nIntroduces starter API for bulk purging of SLI data. \n**docs coming pending review**\nUsers can submit a list of SLO ids for which they would like to purge\nrollup data, as well as a purge policy:\n\n```\n \"purgePolicy\": {\n \"purgeType\": \"fixed_age\",\n \"age\": \"1w\"\n }\n```\n\n```\n \"purgePolicy\": {\n \"purgeType\": \"fixed_time\",\n \"age\": \"2025-04-01T00:00:00Z\"\n }\n```\n\n---------\n\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"2df2e7890698677b617660a0919a3d277add6fb4","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport missing","Team:obs-ux-management","backport:version","v9.1.0","v8.19.0"],"title":"[SLO] Bulk Purge SLI data","number":218287,"url":"https://github.com/elastic/kibana/pull/218287","mergeCommit":{"message":"[SLO] Bulk Purge SLI data (#218287)\n\n## Summary\n\nResolves #210025 \n\nIntroduces starter API for bulk purging of SLI data. \n**docs coming pending review**\nUsers can submit a list of SLO ids for which they would like to purge\nrollup data, as well as a purge policy:\n\n```\n \"purgePolicy\": {\n \"purgeType\": \"fixed_age\",\n \"age\": \"1w\"\n }\n```\n\n```\n \"purgePolicy\": {\n \"purgeType\": \"fixed_time\",\n \"age\": \"2025-04-01T00:00:00Z\"\n }\n```\n\n---------\n\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"2df2e7890698677b617660a0919a3d277add6fb4"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/218287","number":218287,"mergeCommit":{"message":"[SLO] Bulk Purge SLI data (#218287)\n\n## Summary\n\nResolves #210025 \n\nIntroduces starter API for bulk purging of SLI data. \n**docs coming pending review**\nUsers can submit a list of SLO ids for which they would like to purge\nrollup data, as well as a purge policy:\n\n```\n \"purgePolicy\": {\n \"purgeType\": \"fixed_age\",\n \"age\": \"1w\"\n }\n```\n\n```\n \"purgePolicy\": {\n \"purgeType\": \"fixed_time\",\n \"age\": \"2025-04-01T00:00:00Z\"\n }\n```\n\n---------\n\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>","sha":"2df2e7890698677b617660a0919a3d277add6fb4"}},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kevin Delemme <kevin.delemme@elastic.co>
## Summary Resolves elastic#210025 Introduces starter API for bulk purging of SLI data. **docs coming pending review** Users can submit a list of SLO ids for which they would like to purge rollup data, as well as a purge policy: ``` "purgePolicy": { "purgeType": "fixed_age", "age": "1w" } ``` ``` "purgePolicy": { "purgeType": "fixed_time", "age": "2025-04-01T00:00:00Z" } ``` --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Resolves #210025
Introduces starter API for bulk purging of SLI data.
docs coming pending review
Users can submit a list of SLO ids for which they would like to purge rollup data, as well as a purge policy: