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.

Sign up for GitHub

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

Jump to bottom

[Log UI] Replace sourceId with mandatory logView prop in LogStream component #134850

Merged
merged 7 commits into from
Jul 4, 2022
Merged

[Log UI] Replace sourceId with mandatory logView prop in LogStream component #134850

merged 7 commits into from
Jul 4, 2022

Conversation

crespocarlos
Copy link
Contributor

Summary

Closes #120930

This PR replaces the optional sourceId with a mandatory logView prop and updates all places that use the LogStream component to pass this new attribute.

For now, usages that didn't explicitly pass the sourceId will pass default in logViewId. This will be changed in follow up PRs

@crespocarlos crespocarlos 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 apm:test-plan-8.4.0 backport:skip This commit does not require backporting v8.4.0 release_note:feature Makes this part of the condensed release notes release_note:enhancement and removed apm:test-plan-8.4.0 release_note:feature Makes this part of the condensed release notes labels Jun 22, 2022
@crespocarlos crespocarlos marked this pull request as ready for review June 22, 2022 10:52
@crespocarlos crespocarlos requested a review from a team June 22, 2022 10:52
@crespocarlos crespocarlos requested review from a team as code owners June 22, 2022 10:52
@elasticmachine
Copy link
Contributor

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

sphilipse
sphilipse approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@sphilipse sphilipse left a comment

Choose a reason for hiding this comment

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

LGTM for Enterprise Search

@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team labels Jun 22, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

efegurkan
efegurkan approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@efegurkan efegurkan left a comment

Choose a reason for hiding this comment

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

Enterprise Search look good

kpollich
kpollich approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Fleet changes LGTM

matschaffer
matschaffer reviewed Jun 22, 2022
View reviewed changes
x-pack/plugins/infra/public/components/log_stream/log_stream.tsx
@@ -64,8 +64,13 @@ export interface LogStreamProps extends LogStreamContentProps {
height?: string | number;
}

interface LogView {
type: 'log-view-reference';
Copy link
Contributor

@matschaffer matschaffer Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What does this 'log-view-reference' string do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, it does nothing 😄 . This PR is part of this Epic #120920 and just prepares the code for what comes next

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. The suspense! 🍿😉

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@matschaffer
Copy link
Contributor

@elasticmachine merge upstream

@matschaffer
Copy link
Contributor

Can someone from @elastic/apm-ui approve this for merge as codeowner?

MiriamAparicio
MiriamAparicio approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@MiriamAparicio MiriamAparicio left a comment •
edited
Loading

Choose a reason for hiding this comment

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

APM changes LGTM 🌟

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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
apm 2.9MB 2.9MB +112.0B
enterpriseSearch 1.5MB 1.5MB +145.0B
fleet 838.4KB 838.4KB +56.0B
infra 1019.5KB 1019.6KB +112.0B
total +425.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 83.9KB 84.0KB +56.0B

History

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

@crespocarlos crespocarlos merged commit 364ab54 into elastic:main Jul 4, 2022
@crespocarlos crespocarlos deleted the 120930-make-config-id-mandatory-in-logstream branch July 4, 2022 13:22
yakhinvadim pushed a commit to yakhinvadim/kibana that referenced this pull request Jul 5, 2022
@crespocarlos @kibanamachine
[Log UI] Replace sourceId with mandatory logView prop in LogStream co…
2ac76ec 
…mponent (elastic#134850)

* replace sourceId with mandatory logview prop in logstream component

* update unit test assert

* revert small change on use_log_view

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sphilipse sphilipse sphilipse approved these changes

@MiriamAparicio MiriamAparicio MiriamAparicio approved these changes

@kpollich kpollich kpollich approved these changes

@efegurkan efegurkan efegurkan approved these changes

@matschaffer matschaffer matschaffer approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Logs UI Logs UI feature release_note:enhancement Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Logs UI] Make configuration ID required in Log Stream component
9 participants
@crespocarlos @elasticmachine @matschaffer @kibana-ci @efegurkan @kpollich @MiriamAparicio @sphilipse @kibanamachine