-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Logs UI / Metrics UI] New Platform migration server followups #51615
Conversation
💔 Build Failed |
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.
Everything looks good. The only thing that stuck out to me is around the aggregation type. It seems like we are defining that list several times so I added some suggestions to try and reduce that some. If you want to just move forward with what you have, just submit for review again and I will approve.
x-pack/legacy/plugins/infra/public/components/metrics_explorer/aggregation.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/common/http_api/metrics_explorer/index.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/components/metrics_explorer/aggregation.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/components/metrics_explorer/aggregation.tsx
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.
I like Chris's dedupe suggestions but otherwise this LGTM
@@ -54,10 +54,6 @@ export function getApmIndicesConfig(config: APMConfig): ApmIndicesConfig { | |||
}; | |||
} | |||
|
|||
// export async function getApmIndices(context: APMRequestHandlerContext) { |
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.
lol sorry, I think this was my fault -- thanks for the clean up :) <3
@simianhacker Thanks for the review and suggestions. I was going to do these in a followup, but they didn't take very long so I've added them in 2139171. If you're happy and can approve I'll merge this 👍 |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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
Summary
This is currently a WIP, but open for visibility.This is a followup PR to #45299, and contains changes that were important, but not blockers in getting the other PR merged.
setup()
method return statement (a918f08) (This isn't used yet, but it just gets us ready for when we move 100% to the NP world / directory).kibanaVersion
property to thekibana.json
manifest for the Infra plugin (b9cb4ee).The changeset here looks a lot bigger than it realistically is. The vast majority of the changes relate to the io-ts conversion for Metrics Explorer. As some of the enums have changed to string unions a lot of files were touched, but for minimal changes.
For testing this PR, please ensure the Metrics portions of the UI still work as expected.
(@elastic/apm-ui have been automatically requested for review as I've removed a comment from an APM file that we left behind in the big migration PR.)