-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
feat(slo): cleanup temp summary documents task #210264
feat(slo): cleanup temp summary documents task #210264
Conversation
5fa8d8e
to
9d1364a
Compare
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
x-pack/solutions/observability/plugins/slo/server/services/tasks/temp_summary_cleanup_task.ts
Show resolved
Hide resolved
526979b
to
6f8394a
Compare
@@ -180,6 +189,8 @@ export class SLOPlugin | |||
?.start(plugins.taskManager, internalSoClient, internalEsClient) | |||
.catch(() => {}); | |||
|
|||
this.tempSummaryCleanupTask?.start({ taskManager: plugins.taskManager }).catch(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally this should only need to be started if user adds SLOs for the first time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or in the case they have some existing SLOs and are only migrating from a previous version? Handling every cases might become tricky.
There is a feature flag that can be used to turn off this task, if someone's complaining about this task and don't use SLOs, it can be disabled easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how can it be disabled in cloud? cloud has an allow list for which kibana configs can be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the question is how can we update that allow list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure myself, on cloud first region you can add all configs but not on other regions. but i think it's highly unlikely it will be needed in this case.
x-pack/solutions/observability/plugins/slo/server/services/tasks/temp_summary_cleanup_task.ts
Outdated
Show resolved
Hide resolved
af95a53
to
46150c8
Compare
335baa9
to
752ce38
Compare
x-pack/solutions/observability/plugins/slo/server/services/tasks/temp_summary_cleanup_task.ts
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/management/clean_up_temp_summary.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/management/clean_up_temp_summary.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/management/clean_up_temp_summary.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/management/clean_up_temp_summary.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/tasks/temp_summary_cleanup_task.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/tasks/temp_summary_cleanup_task.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/tasks/temp_summary_cleanup_task.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/observability/plugins/slo/server/services/management/clean_up_temp_summary.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with suggestions !!
IMO all those log.info should be log.debug, it's an unnecessary info for typical kibana user.
Thanks @shahzad31, I'll update the logger level used and the terminate after 👍🏻 |
86c3927
to
9f06a98
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Response Ops changes LGTM. New task type with 1h
cadence and 2m
timeout.
Resolves #210194
🏇🏻 Summary
These temporary documents are primarily used to return a placeholder SLO Instance after it has been created in the UI, until the two layers of transform generate one or more actual SLO Instances for that SLO Definition. However, we deduplicate these documents only when both a temporary and a non-temporary summary document are returned in the same search query. This situation can never occur if there are more SLO Instances than the page size, resulting in temporary SLO Instances never being deleted.
This PR introduces a new task that runs every hour to delete temporary summary documents that are no longer needed, as real summary documents have been generated by the transforms. As a circuit breaker, we count the number of temp summary documents and return early if nothing is found.