-
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): add reset api #170473
feat(slo): add reset api #170473
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
e31f3cc
to
969ec8c
Compare
… src/core/server/integration_tests/ci_checks'
@@ -38,6 +37,21 @@ export const slo: SavedObjectsType = { | |||
name: SO_SLO_TYPE, | |||
hidden: false, | |||
namespaceType: 'multiple-isolated', | |||
switchToModelVersionAt: '8.10.0', |
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 tried setting this to 8.12.0 but the framework was complaining.
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.
You effectively don't need to set this option, as it gets applied with the default/max value (8.10.0
) if unspecified . It was exposed for type owners that wanted to switch before 8.10.0
(and for testing purposes)
try { | ||
await this.transformManager.install(slo); | ||
} catch (err) { | ||
// what can we do in this case? | ||
throw err; | ||
} |
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 think throwing the error is the best thing we can do at this point. As long as we bubble up the actual error message from ES so they can understand what the problem is. My guess is that if this failed (missing transform permissions OR missing transform node) the two previous calls (stop
and uninstall
) would have failed too.
Maybe we don't need the try/catch
since we're just going to throw at this point and there is nothing to rollback. The SLO model version will still be 1
, so once they've resolved the issue that prevented the install call, they could just reset
again.
Pinging @elastic/actionable-observability (Team: Actionable Observability) |
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
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.
slo
SO type changes LGTM
@@ -38,6 +37,21 @@ export const slo: SavedObjectsType = { | |||
name: SO_SLO_TYPE, | |||
hidden: false, | |||
namespaceType: 'multiple-isolated', | |||
switchToModelVersionAt: '8.10.0', |
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.
You effectively don't need to set this option, as it gets applied with the default/max value (8.10.0
) if unspecified . It was exposed for type owners that wanted to switch before 8.10.0
(and for testing purposes)
export const SLO_MODEL_VERSION = 2; | ||
export const SLO_RESOURCES_VERSION = 2; | ||
export const SLO_SUMMARY_TRANSFORMS_VERSION = 3; |
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.
@simianhacker Let's sync with the value we want to use. So we don't have different version to keep track
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.
Good question, whatever value we use, any time that changes, it will trigger the need to reset
the SLO definitions. What's the harm in leaving it "as is" other than the complexity of having three values?
What if we added a comment to above SLO_MODEL_VERSION
that says:
Anytime we increment
SLO_MODEL_VERSION
, this will notify the user to "RESET" their SLO definitions which in turn re-indexes the SLI values and SLO summaries. Be judicious about changing this value.
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: cc @kdelemme |
This reverts commit af1ad47.
Resolves #170455
🌮 Summary
This PR introduces the reset api for individual SLOs. This API is used to fully reset (aka a factory reset) an SLO. It deletes the related transform if it exists, and recreates it. It also delete the rollup and summary documents related to that SLO (included all revision and all instance ids)