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] Redirect Logs UI to Discover when in serverless mode #154145

Conversation

tonyghiani
Copy link
Contributor

@tonyghiani tonyghiani commented Mar 31, 2023

📓 Summary

Closes #153890

The implementation creates a new LogsApp service where we should keep any logic concerned with what target_app parameter is configured and the actions related to a specific configuration. I thought it could be a good approach to avoid drilling down the global config till we need it and keep it cleaner by injecting only the service with predefined actions.

In this first case, we create a redirect to discover using its locator, and the exposed method can be used anywhere across the app for triggering the redirect.

🧪 Testing

Normal behaviour

When Kibana is used as always, we want to keep the current behaviour and the user will stay on the Logs UI pages.

  • Launch the Kibana dev environment with yarn start
  • Navigate to Logs UI
  • Verify the navigation works normally and that no redirect to Discover occurs

Serverless behaviour

When Kibana is used in serverless mode, we want to redirect any user landing to Logs UI to the Discover page, configuring the same data view or creating an ad-hoc one starting from the index pattern

  • Launch the Kibana dev environment with yarn serverless-oblt
  • Navigate to Logs UI
  • Verify to be redirected to Discover and a temporary data view is created from the current index pattern

@github-actions
Copy link
Contributor

Documentation preview:

@apmmachine
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 elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@tonyghiani tonyghiani added 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 Apr 4, 2023
@tonyghiani tonyghiani marked this pull request as ready for review April 12, 2023 10:40
@tonyghiani tonyghiani requested review from a team as code owners April 12, 2023 10:40
@elasticmachine
Copy link
Contributor

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

@weltenwort weltenwort self-requested a review April 13, 2023 10:33
Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

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

@tonyghiani I noticed when running in serverless mode the nav link to logs in the main navigation sidebar is gone. If I navigate to Observability and then to any of the 3 Logs nav links (Stream, Anomalies, Categories), I am redirected to Discover as expected. However, using the browser back button will not take me back to Observability. Is this the intended behavior?

@weltenwort
Copy link
Member

weltenwort commented Apr 13, 2023

@jeramysoucy yes, we're intentionally hiding the Logs UI from the serverless deployment. The plan is to use a Discover variant that has been enhanced for log consumption instead. The observability navigation will also not exist as it does right now. How and where that new Discover-base log explorer will be represented in the solution navigation is still undecided, because we're still waiting for the basic implementation of that navigation.

@weltenwort
Copy link
Member

@jeramysoucy I only just realized what you were asking specifically. Sorry for not reading carefully enough. 🙈 You're right that the redirect creates a new browser history entry instead of replacing the current one. Thanks for pointing that out! ❤️

@tonyghiani the locator's navigate() call supports a second argument in which we can specify that it replaces the history entry. That would make sense to me. Would you agree?

@tonyghiani
Copy link
Contributor Author

@tonyghiani the locator's navigate() call supports a second argument in which we can specify that it replaces the history entry. That would make sense to me. Would you agree?

Totally, I'll update that part too!

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.

This looks really good, just one more question arose while testing:

const { discover } = plugins;
const { logViews } = pluginStart;

const machine = createLogViewStateMachine({
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use the log view reference from the url instead of a hard-coded default here? Or am I missing some other mechanism using which it is initialized correctly?

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 didn't think of it and you are right, it was always using the default log view, even with a different one specified in the URL. I created the initializeFromUrl function and passed it to the state machine, now it does correctly recover the URL param if exists.

Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

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

Looks good from AppEx Security perspective, and nav history works as expected now. 👍

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.

Well done, looks like great quality to me 👍 thanks for investing the effort!

@tonyghiani tonyghiani enabled auto-merge (squash) April 19, 2023 14:28
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1339 1340 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
infra 40 41 +1

Async chunks

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

id before after diff
infra 2.0MB 2.0MB +1.5KB

Page load bundle

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

id before after diff
infra 90.1KB 90.5KB +407.0B
Unknown metric groups

API count

id before after diff
infra 43 44 +1

async chunk count

id before after diff
infra 28 29 +1

ESLint disabled line counts

id before after diff
securitySolution 394 397 +3

Total ESLint disabled count

id before after diff
securitySolution 474 477 +3

History

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

@tonyghiani tonyghiani merged commit b40b89e into elastic:main Apr 19, 2023
@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 19, 2023
@tonyghiani tonyghiani deleted the 153890-redirect-logs-ui-to-discover-serverless branch April 19, 2023 15:34
criamico added a commit that referenced this pull request Jul 10, 2024
Closes #185711

## Summary
This change fixes #185711, but
while working on that I also realised that we should move away from
using hardcoded urls. So this PR does two things:
- Displays the button only when the user has `authz.fleet.readAgents`
privilege
- Refactors the button functionality to use the new locators that take
care of linking to the observability logs/discover app

### Why the refactor
While testing this button, I noticed that the functionality was broken
in some cases, that's because we were manually routing the urls to Logs
UI/Discover apps based if we are in serverless or not.

I found a PR that already implements this functionality:
#155156
I also found #154145 that takes
care of the redirect to the correct app.
So I'm replacing the current manual functionality with these utilities
so that `getLogsLocatorsFromUrlService` takes care of where the open in
logs button should link.



### ESS


https://github.com/elastic/kibana/assets/16084106/3f0760c9-3afb-4793-a3af-317f625b36d7


https://github.com/elastic/kibana/assets/16084106/3436cf5a-36c9-425d-a114-e116ddaa1a03

### Serverless

https://github.com/elastic/kibana/assets/16084106/84176f09-96a4-4932-9508-5f7682d03aae

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 10, 2024
…ic#187871)

Closes elastic#185711

## Summary
This change fixes elastic#185711, but
while working on that I also realised that we should move away from
using hardcoded urls. So this PR does two things:
- Displays the button only when the user has `authz.fleet.readAgents`
privilege
- Refactors the button functionality to use the new locators that take
care of linking to the observability logs/discover app

### Why the refactor
While testing this button, I noticed that the functionality was broken
in some cases, that's because we were manually routing the urls to Logs
UI/Discover apps based if we are in serverless or not.

I found a PR that already implements this functionality:
elastic#155156
I also found elastic#154145 that takes
care of the redirect to the correct app.
So I'm replacing the current manual functionality with these utilities
so that `getLogsLocatorsFromUrlService` takes care of where the open in
logs button should link.

### ESS

https://github.com/elastic/kibana/assets/16084106/3f0760c9-3afb-4793-a3af-317f625b36d7

https://github.com/elastic/kibana/assets/16084106/3436cf5a-36c9-425d-a114-e116ddaa1a03

### Serverless

https://github.com/elastic/kibana/assets/16084106/84176f09-96a4-4932-9508-5f7682d03aae

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 696bb88)
kibanamachine added a commit that referenced this pull request Jul 10, 2024
…#187871) (#188000)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[Fleet] Display view in logs button when logs app is available
(#187871)](#187871)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Cristina
Amico","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-07-10T15:17:31Z","message":"[Fleet]
Display view in logs button when logs app is available
(#187871)\n\nCloses
https://github.com/elastic/kibana/issues/185711\r\n\r\n##
Summary\r\nThis change fixes
#185711, but\r\nwhile working on
that I also realised that we should move away from\r\nusing hardcoded
urls. So this PR does two things:\r\n- Displays the button only when the
user has `authz.fleet.readAgents`\r\nprivilege\r\n- Refactors the button
functionality to use the new locators that take\r\ncare of linking to
the observability logs/discover app\r\n\r\n### Why the refactor\r\nWhile
testing this button, I noticed that the functionality was broken\r\nin
some cases, that's because we were manually routing the urls to
Logs\r\nUI/Discover apps based if we are in serverless or not.\r\n\r\nI
found a PR that already implements this
functionality:\r\nhttps://github.com//pull/155156\r\nI
also found #154145 that
takes\r\ncare of the redirect to the correct app.\r\nSo I'm replacing
the current manual functionality with these utilities\r\nso that
`getLogsLocatorsFromUrlService` takes care of where the open in\r\nlogs
button should link.\r\n\r\n\r\n\r\n###
ESS\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/3f0760c9-3afb-4793-a3af-317f625b36d7\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/3436cf5a-36c9-425d-a114-e116ddaa1a03\r\n\r\n###
Serverless\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/84176f09-96a4-4932-9508-5f7682d03aae\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"696bb88d7c33eebfeabec6064ea8a97a2e2bb1bb","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Fleet","v8.15.0","v8.16.0"],"title":"[Fleet]
Display view in logs button when logs app is
available","number":187871,"url":"https://github.com/elastic/kibana/pull/187871","mergeCommit":{"message":"[Fleet]
Display view in logs button when logs app is available
(#187871)\n\nCloses
https://github.com/elastic/kibana/issues/185711\r\n\r\n##
Summary\r\nThis change fixes
#185711, but\r\nwhile working on
that I also realised that we should move away from\r\nusing hardcoded
urls. So this PR does two things:\r\n- Displays the button only when the
user has `authz.fleet.readAgents`\r\nprivilege\r\n- Refactors the button
functionality to use the new locators that take\r\ncare of linking to
the observability logs/discover app\r\n\r\n### Why the refactor\r\nWhile
testing this button, I noticed that the functionality was broken\r\nin
some cases, that's because we were manually routing the urls to
Logs\r\nUI/Discover apps based if we are in serverless or not.\r\n\r\nI
found a PR that already implements this
functionality:\r\nhttps://github.com//pull/155156\r\nI
also found #154145 that
takes\r\ncare of the redirect to the correct app.\r\nSo I'm replacing
the current manual functionality with these utilities\r\nso that
`getLogsLocatorsFromUrlService` takes care of where the open in\r\nlogs
button should link.\r\n\r\n\r\n\r\n###
ESS\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/3f0760c9-3afb-4793-a3af-317f625b36d7\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/3436cf5a-36c9-425d-a114-e116ddaa1a03\r\n\r\n###
Serverless\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/84176f09-96a4-4932-9508-5f7682d03aae\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"696bb88d7c33eebfeabec6064ea8a97a2e2bb1bb"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"8.15","label":"v8.15.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/187871","number":187871,"mergeCommit":{"message":"[Fleet]
Display view in logs button when logs app is available
(#187871)\n\nCloses
https://github.com/elastic/kibana/issues/185711\r\n\r\n##
Summary\r\nThis change fixes
#185711, but\r\nwhile working on
that I also realised that we should move away from\r\nusing hardcoded
urls. So this PR does two things:\r\n- Displays the button only when the
user has `authz.fleet.readAgents`\r\nprivilege\r\n- Refactors the button
functionality to use the new locators that take\r\ncare of linking to
the observability logs/discover app\r\n\r\n### Why the refactor\r\nWhile
testing this button, I noticed that the functionality was broken\r\nin
some cases, that's because we were manually routing the urls to
Logs\r\nUI/Discover apps based if we are in serverless or not.\r\n\r\nI
found a PR that already implements this
functionality:\r\nhttps://github.com//pull/155156\r\nI
also found #154145 that
takes\r\ncare of the redirect to the correct app.\r\nSo I'm replacing
the current manual functionality with these utilities\r\nso that
`getLogsLocatorsFromUrlService` takes care of where the open in\r\nlogs
button should link.\r\n\r\n\r\n\r\n###
ESS\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/3f0760c9-3afb-4793-a3af-317f625b36d7\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/3436cf5a-36c9-425d-a114-e116ddaa1a03\r\n\r\n###
Serverless\r\n\r\nhttps://github.com/elastic/kibana/assets/16084106/84176f09-96a4-4932-9508-5f7682d03aae\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"696bb88d7c33eebfeabec6064ea8a97a2e2bb1bb"}}]}]
BACKPORT-->

Co-authored-by: Cristina Amico <[email protected]>
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 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] Redirect Logs UI routes to Discover when serverless
7 participants