-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Infra UI] Fix Open in Logs
links in Infra and APM when in Serverless
#172137
[Infra UI] Fix Open in Logs
links in Infra and APM when in Serverless
#172137
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
…l fall back to `AllDataSetsLocator` when infra locators aren't available.
7e8602d
to
573d568
Compare
/oblt-deploy |
/oblt-deploy-serverless |
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obs UX Changes LGTM !!
Pinging @elastic/apm-ui (Team:APM) |
/oblt-deploy |
/oblt-deploy-serverless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the fix.
import { NODE_LOGS_LOCATOR_ID } from './node_logs_locator'; | ||
import { TRACE_LOGS_LOCATOR_ID } from './trace_logs_locator'; | ||
|
||
export const getLogsLocatorsFromUrlService = (urlService: UrlService) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
I am a bit confused about the need for this, isn't this the same as the locators exposed from the plugin setup contract?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This serves just as a wrapper function to get logs_shared locators exposed in the logs_shared plugin setup. The benefit is to not refer the locator ids directly in the consumer plugins (infra, apm). The wrapper function will also help to not change the consumer code if something changes with the way the locators are returned in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, I just think that even with the returned locators from the plugin contract you will still not need the ids themselves. We can maybe keep the wrapper you have but in this case I don't think we need to return the locators right? I mean one of them should be enough
…-logs-locators # Conflicts: # x-pack/plugins/apm/public/components/shared/transaction_action_menu/transaction_action_menu.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Work!! Thanks for fixing this LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
…ss (elastic#172137) Fixes elastic#171082 ## Summary The PR wraps the [LogsLocator](https://github.com/elastic/kibana/blob/f59ac2916d02d643310dd44a8f80b7a9cc61f608/x-pack/plugins/infra/common/locators/logs_locator.ts#L18C27-L18C27) and [NodeLogsLocator](https://github.com/elastic/kibana/blob/f59ac2916d02d643310dd44a8f80b7a9cc61f608/x-pack/plugins/infra/common/locators/node_logs_locator.ts#L16) of **infra** plugin inside corresponding locators in **logs_shared** plugin while including the fallback logic to navigate to Logs Explorer when Steam UI isn't available. Previously, it was assumed that Steam UI will always be available as long as Infra UI is available, but **infra** plugin introduced a new feature flag `logsUIEnabled` which when `false` won't enable the `/stream/` route in Serverless. The added locators in **logs_shared** will now check whether locators redirecting to `/steam/` are available, otherwise they'll redirect to Logs Explorer, thus the new locators are abstracting this decision in their definition. This abstraction was already being done in **apm** plugin, which has also been refactored to use the newly added **logs_shared** locators. Links in Serverless: https://github.com/elastic/kibana/assets/2748376/16e5747a-546e-44b3-87e3-95428945cf63
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
3 similar comments
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Fixes #171082
Summary
The PR wraps the LogsLocator and NodeLogsLocator of infra plugin inside corresponding locators in logs_shared plugin while including the fallback logic to navigate to Logs Explorer when Steam UI isn't available.
Previously, it was assumed that Steam UI will always be available as long as Infra UI is available, but infra plugin introduced a new feature flag
logsUIEnabled
which whenfalse
won't enable the/stream/
route in Serverless.The added locators in logs_shared will now check whether locators redirecting to
/steam/
are available, otherwise they'll redirect to Logs Explorer, thus the new locators are abstracting this decision in their definition. This abstraction was already being done in apm plugin, which has also been refactored to use the newly added logs_shared locators.Links in Serverless:
Open-in-Logs_link_Serverless.mov