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

[embeddable rebuild] log stream react embeddable #184247

Merged
merged 15 commits into from
Jun 7, 2024
Merged

[embeddable rebuild] log stream react embeddable #184247

merged 15 commits into from
Jun 7, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented May 24, 2024 •
edited
Loading

PR migrates log stream embeddable from the legacy class based system.

test instructions

  1. Run kibana on a system with o11y data and log streams
  2. Create a new dashboard, click "Add panel" => "Log stream"
  3. Verify panel behavior has not changed with legacy embeddable
  4. Click panel context menu and select "Settings"
  5. Set custom title, description and time range. Verify behavior has not changed with legacy embeddable
  6. Import dashboard with log stream panel. Verify behavior has not changed with legacy embeddable

@nreese
Copy link
Contributor Author

nreese commented May 24, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented May 24, 2024

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented May 24, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented May 24, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented May 24, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented May 24, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented May 24, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented May 24, 2024

/ci

@nreese nreese marked this pull request as ready for review May 25, 2024 00:51
@nreese nreese requested review from a team as code owners May 25, 2024 00:51
@nreese nreese added release_note:skip Skip the PR/issue when compiling release notes project:embeddableRebuild v8.15.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels May 25, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

tonyghiani
tonyghiani reviewed May 27, 2024
View reviewed changes
Copy link
Contributor

@tonyghiani tonyghiani 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.

The changes LGTM, although I have a suggestion which would make easier the cleanup and imminent deprecation of the LogStream:
as this embeddable has nothing concerning the infra plugin and all the logic for the <LogStream /> component lives in the logs_shared plugin, can we do the extra mile and move this embeddable and it feature registration into logs_shared?

I know it's unplanned work and is not required for these changes, although it would make related pieces closer and have a clearer ownership of this in the codebase (+ when deprecating this, all the related parts would be together in the same plugin).

Let me know your thoughts and feel free to reach out with any questions 👌

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label May 27, 2024
@Heenawter Heenawter linked an issue May 28, 2024 that may be closed by this pull request
[Embeddables Rebuild] Migrate Log Stream #174969
Closed
@nreese
Copy link
Contributor Author

nreese commented Jun 5, 2024

The changes LGTM, although I have a suggestion which would make easier the cleanup and imminent deprecation of the LogStream:
as this embeddable has nothing concerning the infra plugin and all the logic for the component lives in the logs_shared plugin, can we do the extra mile and move this embeddable and it feature registration into logs_shared?

I know it's unplanned work and is not required for these changes, although it would make related pieces closer and have a clearer ownership of this in the codebase (+ when deprecating this, all the related parts would be together in the same plugin).

Let me know your thoughts and feel free to reach out with any questions 👌

@tonyghiani

I would like to keep the scoping of this PR as minimal as possible and would prefer to exclude all unnecessary work at this time.

@tonyghiani
Copy link
Contributor

I would like to keep the scoping of this PR as minimal as possible and would prefer to exclude all unnecessary work at this time.

Ok, I'll take another round of review to double-check 👌

@nreese nreese requested a review from tonyghiani June 5, 2024 15:10
tonyghiani
tonyghiani approved these changes Jun 6, 2024
View reviewed changes
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.

Code LGTM, can the legacy log_stream_embeddable.ts file be removed or we still need it?

jennypavlova
jennypavlova approved these changes Jun 6, 2024
View reviewed changes
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

Infra plugin config code changes LGTM

x-pack/plugins/observability_solution/infra/public/plugin.ts Outdated Show resolved Hide resolved
@nreese
Copy link
Contributor Author

nreese commented Jun 6, 2024

can the legacy log_stream_embeddable.ts file be removed or we still need it?

Thanks for pointing this out. I have removed the file with 46e8209

ThomThomson
ThomThomson approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Presentation team changes LGTM! Code only review. Looks like a really straightforward embeddable conversion!

@nreese
Copy link
Contributor Author

nreese commented Jun 7, 2024

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jun 7, 2024 •
edited
Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1570 1612 +42

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.5MB 1.6MB +9.8KB

Page load bundle

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

id before after diff
infra 101.7KB 101.8KB +122.0B
Unknown metric groups

async chunk count

id before after diff
infra 17 18 +1

References to deprecated APIs

id before after diff
infra 10 9 -1

History

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

@nreese nreese merged commit 1a0b93a into elastic:main Jun 7, 2024
20 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 7, 2024
eokoneyo pushed a commit to eokoneyo/kibana that referenced this pull request Jun 13, 2024
@nreese @kibanamachine @eokoneyo
[embeddable rebuild] log stream react embeddable (elastic#184247)
be3d34a 
PR migrates log stream embeddable from the legacy class based system.

### test instructions
1. Run kibana on a system with o11y data and log streams
2. Create a new dashboard, click "Add panel" => "Log stream"
3. Verify panel behavior has not changed with legacy embeddable
4. Click panel context menu and select "Settings"
5. Set custom title, description and time range. Verify behavior has not
changed with legacy embeddable
6. Import dashboard with log stream panel. Verify behavior has not
changed with legacy embeddable

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonyghiani tonyghiani tonyghiani approved these changes

@ThomThomson ThomThomson ThomThomson approved these changes

@jennypavlova jennypavlova jennypavlova approved these changes

Assignees
No one assigned
Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Embeddables Rebuild] Migrate Log Stream
8 participants
@nreese @elasticmachine @tonyghiani @kibana-ci @jennypavlova @ThomThomson @smith @kibanamachine