Skip to content
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] Support inline Log Views in routes #151760

Merged
merged 10 commits into from
Mar 2, 2023

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Feb 21, 2023

Summary

Closes #151489

This is the server side portion of support for transient / inline Log Views. Alerting and ML based functionality is scoped to the persisted type only, as we won't be supporting inline Log Views in those contexts.

In terms of UI, changes have been made as close to the edge / boundary to the server (e.g. where we actually make network requests) as possible. This is because the bulk of the UI changes will come in #142840.

Testing

This is predominantly a type-driven refactoring (the best kind), so we should be relatively safe here (especially since inline views aren't in the UI yet). But, a quick test of each page and a high level functional test would still be good.

@Kerry350 Kerry350 self-assigned this Feb 22, 2023
@Kerry350 Kerry350 added Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes labels Feb 22, 2023
@Kerry350 Kerry350 marked this pull request as ready for review February 22, 2023 16:10
@Kerry350 Kerry350 requested a review from a team as a code owner February 22, 2023 16:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

Copy link
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as you suggested I went through the pages and everything works. Thanks also for splitting and renaming the existing logViewReferenceRT into persistedLogViewReferenceRT and inlineLogViewReferenceRT, makes much more sense in terms of naming!

I'm wondering if renaming the type for the persistedLogReference to log-view-persisted would make sense to keep the naming consistent with the new log-view-inline type, but I guess this could break the existing saved objects and require a migration right?

@Kerry350
Copy link
Contributor Author

Kerry350 commented Mar 2, 2023

@tonyghiani

Thanks for reviewing.

I'm wondering if renaming the type for the persistedLogReference to log-view-persisted would make sense to keep the naming consistent with the new log-view-inline type, but I guess this could break the existing saved objects and require a migration right?

I pondered a similar change myself, but left it for now. It won't affect the Saved Object which is nice. The type here on the LogViewReference is purely for us to make the top level differentiation in our code, it doesn't get stored on the SO itself. The Saved Object does have a type, but that's infrastructure-monitoring-log-view (or infrastructure-ui-source for the legacy fallback), and that's our actual lookup for finding instances of the correct type of SO. It can be confusing, but I hope that helps a bit.

I'll hold off on any renaming here, purely because the UI work is branched off of here so it'd be nice to get it merged. We might actually be able to consider it as part of #152278.

@tonyghiani
Copy link
Contributor

I pondered a similar change myself, but left it for now. It won't affect the Saved Object which is nice. The type here on the LogViewReference is purely for us to make the top level differentiation in our code, it doesn't get stored on the SO itself. The Saved Object does have a type, but that's infrastructure-monitoring-log-view (or infrastructure-ui-source for the legacy fallback), and that's our actual lookup for finding instances of the correct type of SO. It can be confusing, but I hope that helps a bit.

I'll hold off on any renaming here, purely because the UI work is branched off of here so it'd be nice to get it merged. We might actually be able to consider it as part of #152278.

Very useful, thanks for clarifying :)

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.3MB 1.3MB +436.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
infra 87.6KB 87.7KB +122.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Kerry350

@Kerry350 Kerry350 added the v8.8.0 label Mar 2, 2023
@Kerry350 Kerry350 merged commit e772fa1 into elastic:main Mar 2, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 2, 2023
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
## Summary

Closes elastic#151489

This is the server side portion of support for transient / inline Log
Views. Alerting and ML based functionality is scoped to the persisted
type only, as we won't be supporting inline Log Views in those contexts.

In terms of UI, changes have been made as close to the edge / boundary
to the server (e.g. where we actually make network requests) as
possible. This is because the bulk of the UI changes will come in
elastic#142840.

## Testing

This is predominantly a type-driven refactoring (the best kind), so we
should be relatively safe here (especially since inline views aren't in
the UI yet). But, a quick test of each page and a high level functional
test would still be good.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Support inline log views in http routes
5 participants