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

[Logs] Fix Logs stream minimap after changes to use @emotion #203620

Closed
wants to merge 4 commits into from
Closed

[Logs] Fix Logs stream minimap after changes to use @emotion #203620

wants to merge 4 commits into from

Conversation

crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Dec 10, 2024 •
edited
Loading

fixes #203597

Summary

Fix an error on the Logs Stream UI after replacing euiStyled with emotion

logs_stream

How to test

  • Enable Logs Stream in the Advanced Settings
  • Navigate to Logs Stream page

@crespocarlos
Copy link
Contributor Author

/ci

@crespocarlos crespocarlos added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 Team:obs-ux-logs Observability Logs User Experience Team Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team backport:version Backport to applied version labels labels Dec 10, 2024
MiriamAparicio
MiriamAparicio approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@MiriamAparicio MiriamAparicio left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, LGTM

@crespocarlos crespocarlos marked this pull request as ready for review December 11, 2024 08:51
@crespocarlos crespocarlos requested a review from a team as a code owner December 11, 2024 08:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@weltenwort weltenwort self-requested a review December 12, 2024 10:48
weltenwort
weltenwort approved these changes Dec 12, 2024
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Thanks for converting this ancient code from pre-function-component times.

...k/plugins/observability_solution/infra/public/components/logging/log_minimap/log_minimap.tsx Outdated
summaryBuckets,
summaryHighlightBuckets,
width,
target,
Copy link
Member

Choose a reason for hiding this comment

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

For a faithful conversion of the original class component we would have to capture the initial target in a useState so it doesn't update after the first render. I'm not sure about the reason anymore, but I think it has something to do with deep links to specific points in time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was confused about that and really didn't see any difference with storing target in a useState, that's why I didn't do it here. target isn't changed in this component.

I'll change it because I don't whether this will have other implications I'm not aware of.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I felt the same. Sorry that I can't say for certain. Better safe than sorry, since it's a trivial change.

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Dec 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@gbamparop
Copy link
Contributor

Thanks for converting this ancient code from pre-function-component times.

@weltenwort isn't there a functional test that covers the log stream?

@weltenwort
Copy link
Member

weltenwort commented Dec 12, 2024 •
edited
Loading

🤔 I don't think so, since it's essentially an SVG curve.

edit: I mean there are tests for the log stream, but not for the minimap specifically.

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@gbamparop
Copy link
Contributor

🤔 I don't think so, since it's essentially an SVG curve.

edit: I mean there are tests for the log stream, but not for the minimap specifically.

AFAIKT this bug breaks the whole page

@weltenwort
Copy link
Member

AFAIKT this bug breaks the whole page

Good point. I'm certain I wrote tests that edit the column settings and check whether they apply to the Logs UI at some point. We'll have to check what happened to them.

@crespocarlos
Copy link
Contributor Author

🤔 I don't think so, since it's essentially an SVG curve.
edit: I mean there are tests for the log stream, but not for the minimap specifically.

AFAIKT this bug breaks the whole page

This should've caught the problem https://github.com/elastic/kibana/blob/main/x-pack/test/functional/apps/infra/logs/log_stream.ts, if I'm not mistaken.

@crespocarlos
Copy link
Contributor Author

The test passed: https://buildkite.com/organizations/elastic/pipelines/kibana-pull-request/builds/257579/jobs/01939806-fb3c-4899-bcf6-8e62e8cc9932/log

Maybe it's something we need to improve or find out why the test passed

@weltenwort
Copy link
Member

Indeed, this is curious. Could it be that emotion fails silently when running against the production build and only throws the error in development mode?

@weltenwort
Copy link
Member

Or maybe it actually works correctly in the production build and only fails in development? The error message indicates a problem with a babel processor, which might be configured differently in the two environments.

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@crespocarlos crespocarlos mentioned this pull request Dec 18, 2024
Merged
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 18, 2024 •
edited
Loading

💚 Build Succeeded

  • Buildkite Build
  • Commit: 8088845
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-203620-80888453d761

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.7MB 1.7MB -374.0B

History

@crespocarlos
Copy link
Contributor Author

I'm closing this PR in favor of #204115. Logs stream is being removed in v9.0, and the bug would only affect v9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weltenwort weltenwort weltenwort approved these changes

@MiriamAparicio MiriamAparicio MiriamAparicio approved these changes

Assignees
No one assigned
Labels
backport:version Backport to applied version labels ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:obs-ux-logs Observability Logs User Experience Team v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ObsUX][Infra] Bug: Error when trying to access to Logs UI locally
5 participants
@crespocarlos @elasticmachine @gbamparop @weltenwort @MiriamAparicio