-
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
[Stack Monitoring] Logstash migration #113256
Conversation
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
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.
Got a couple code process notes. Looks decent overall. I'll try to exercise it.
x-pack/plugins/monitoring/public/components/logstash/overview/index.d.ts
Outdated
Show resolved
Hide resolved
We should probably add the single node view as well, or remove the "fixes" from the description here |
871beae
to
d556309
Compare
@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.
Spotted a couple of issues. But we could potentially fix in a follow up.
x-pack/plugins/monitoring/public/application/pages/logstash/logstash_template.tsx
Show resolved
Hide resolved
x-pack/plugins/monitoring/public/application/pages/logstash/pipeline.tsx
Show resolved
Hide resolved
x-pack/plugins/monitoring/public/application/pages/logstash/pipeline.tsx
Outdated
Show resolved
Hide resolved
if (pageData === null) { | ||
return null; | ||
} | ||
const { clusterStatus, pipelines } = pageData || {}; |
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.
nit: the fallback to an empty object breaks line 74 when reading clusterStatus.versions
. Can we make the initial pageData
check more robust (or maybe null is enough) and get rid of fallbacks and conditionals in the following lines ?
… into logstash-migration
… into logstash-migration
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/search/session·ts.apis search search session touched time updates when you poll on an searchStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
History
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.
I think we can go with this. I noticed a few code level tweaks that I put in phillipb#1 for you (just merge if you're cool with it).
The other weird thing I notices is that the pipeline view detail drawer re-opens after you hit the "X" then you have to click it again to close. Here's a recording:
Otherwise looks good to me and we can patch up weirdness as a followup I think.
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Phillip Burch <[email protected]>
Summary
Migrate logstash pages. Fixes: #111759