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

[Infra UI] Fix Open in Logs links in Infra and APM when in Serverless #172137

Merged
merged 22 commits into from
Jan 10, 2024
Merged

[Infra UI] Fix Open in Logs links in Infra and APM when in Serverless #172137

merged 22 commits into from
Jan 10, 2024

Conversation

awahab07
Copy link
Contributor

@awahab07 awahab07 commented Nov 29, 2023 •
edited by kibanamachine
Loading

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 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:

Open-in-Logs_link_Serverless.mov

@awahab07 awahab07 added the bug Fixes for quality problems that affect the customer experience label Nov 29, 2023
@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.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@awahab07 awahab07 force-pushed the 171082-refactor-infra-logs-locators branch from 7e8602d to 573d568 Compare December 3, 2023 17:06
@awahab07
Copy link
Contributor Author

awahab07 commented Dec 4, 2023

/oblt-deploy

@awahab07
Copy link
Contributor Author

awahab07 commented Dec 4, 2023

/oblt-deploy-serverless

@awahab07 awahab07 added Team:obs-ux-logs Observability Logs User Experience Team release_note:fix v8.12.0 labels Dec 5, 2023
@awahab07 awahab07 marked this pull request as ready for review December 5, 2023 14:05
@awahab07 awahab07 requested review from a team as code owners December 5, 2023 14:05
@elasticmachine
Copy link
Contributor

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

shahzad31
shahzad31 approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@shahzad31 shahzad31 left a 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 !!

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Dec 5, 2023
@elasticmachine
Copy link
Contributor

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

@jennypavlova jennypavlova self-requested a review December 5, 2023 14:15
@awahab07
Copy link
Contributor Author

/oblt-deploy

@awahab07
Copy link
Contributor Author

/oblt-deploy-serverless

jennypavlova
jennypavlova approved these changes Jan 3, 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.

LGTM! Thanks for the fix.

mohamedhamed-ahmed
mohamedhamed-ahmed reviewed Jan 4, 2024
View reviewed changes
x-pack/plugins/logs_shared/common/locators/get_logs_locators.ts
import { NODE_LOGS_LOCATOR_ID } from './node_logs_locator';
import { TRACE_LOGS_LOCATOR_ID } from './trace_logs_locator';

export const getLogsLocatorsFromUrlService = (urlService: UrlService) => {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

@awahab07
Merge remote-tracking branch 'origin/main' into 171082-refactor-infra…
01f5745 
…-logs-locators

# Conflicts:
#	x-pack/plugins/apm/public/components/shared/transaction_action_menu/transaction_action_menu.tsx
@awahab07
Merge fix.
1da85c6
mohamedhamed-ahmed
mohamedhamed-ahmed approved these changes Jan 6, 2024
View reviewed changes
Copy link
Contributor

@mohamedhamed-ahmed mohamedhamed-ahmed left a 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

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1547 1546 -1
logsShared 207 217 +10
total +9

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
logsShared 276 287 +11

Async chunks

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

id before after diff
apm 3.1MB 3.1MB -170.0B
infra 1.3MB 1.3MB +85.0B
total -85.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
logsShared 26 27 +1

Page load bundle

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

id before after diff
apm 34.8KB 34.9KB +147.0B
infra 99.5KB 99.7KB +206.0B
logsShared 219.1KB 222.1KB +3.0KB
total +3.3KB
Unknown metric groups

API count

id before after diff
logsShared 291 302 +11

ESLint disabled line counts

id before after diff
logsShared 16 15 -1

Total ESLint disabled count

id before after diff
logsShared 19 18 -1

History

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

@awahab07 awahab07 merged commit 2f2083b into elastic:main Jan 10, 2024
20 checks passed
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.12 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 172137

Questions ?

Please refer to the Backport tool documentation

delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
@awahab07 @delanni
[Infra UI] Fix Open in Logs links in Infra and APM when in Serverle…
8c14f93 
…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
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 11, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 172137 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 172137 locally

@kibanamachine kibanamachine mentioned this pull request Jan 24, 2024
Merged
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 172137 locally

3 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 172137 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 172137 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 172137 locally

@jbudz jbudz added the backport:skip This commit does not require backporting label Sep 30, 2024
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

@shahzad31 shahzad31 shahzad31 approved these changes

@jloleysens jloleysens jloleysens approved these changes

@mohamedhamed-ahmed mohamedhamed-ahmed mohamedhamed-ahmed approved these changes

@jennypavlova jennypavlova jennypavlova approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience release_note:fix Team:APM All issues that need APM UI Team support Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:obs-ux-logs Observability Logs User Experience Team v8.12.0 v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Infra UI] Serverless 'Open in Logs' link within Host fly-out is broken