-
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
New slo plugin #177937
New slo plugin #177937
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
A documentation preview will be available soon. Request a new doc build by commenting
If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here. |
997721d
to
0ab9dda
Compare
8368ce0
to
513ab04
Compare
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.
Changes to observability navigation_tree.ts
LGTM 👍
@elasticmachine merge upstream |
@CoenWarmer regarding handling redirection of the old slo detail and edit pages, here's what I've done and works fine. If you know any other better out of the box solution, just let me know Screen.Recording.2024-03-18.at.13.30.38.mov |
@@ -155,4 +159,18 @@ export const routes = { | |||
params: {}, | |||
exact: true, | |||
}, | |||
[OLD_SLO_DETAIL_PATH]: { | |||
handler: () => { | |||
return <SimpleRedirect to="/:sloId" redirectToApp="slo" />; |
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.
Don't you need something before /:sloId
? How does the <SimpleRedirect />
component distinguish between the edit and detail route? The to
prop is the same in both cases?
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.
@CoenWarmer I handle both edit and detail routes in this condition, which takes out the /slos
part from the url and keeps /edit/1234
or /1234
accordingly:
if (to === '/:sloId') {
to = pathname.split('/slos')[1];
}
Initially I added this condition just for the detail path, but then it worked out well for the edit as well. What do you think? I agree not the most elegant way. I am wondering if React router has something out of the box, that you are aware of.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Smoke tests LGTM
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled in files
ESLint disabled line counts
miscellaneous assets size
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @mgiota |
Fixes #176420
🍒 Summary
This PR copies the SLO code that was inside the Observability app into its own app under
observability-solution/slo
folder.Screen.Recording.2024-03-09.at.00.45.40.mov
✔️ Acceptance criteria
app/slos
x-pack/plugins/observability_solution/slo
.observability_solution/observability
code has been removed. A new clean up round might be needed though for possible leftovers.ApplicationUsageTrackingProvider
which will send sloApplication usage
information tracked by theslo
appIdapp/observability/slos
route toapp/slos
xpack.observability.slo
keys toxpack.slo
in the translation files🌮 How to test
Design and functionality didn't change, so simply navigate to existing slo pages and try to break it
TODO