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 UX] Classify logs-data-access and logs-shared as platform plugins #201263

Merged
merged 19 commits into from
Dec 17, 2024

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Nov 21, 2024

📝 Summary

This classifies @kbn/logs-data-access-plugin, @kbn/logs-shared-plugin and @kbn/observability-logs-overview as "platform/shared".

🕵️‍♀️ Review Notes

In order to remove the forbidden dependency of logs-shared on observability-shared a few changes were made:

  • The link to APM is created via the locator registry. The existing transaction-by-trace-id locator was enhanced to support passing the time range parameters as required.
  • The ability to extend the log entry context menu via the triggers mechanism was removed, because it was unused anyway.
  • Some convenience wrappers for the ui counters were inlined.
  • @kbn/xstate-utils as a shared dependency was also classified as platform/shared.

@weltenwort weltenwort added the Team:obs-ux-logs Observability Logs User Experience Team label Nov 21, 2024
@weltenwort weltenwort self-assigned this Nov 21, 2024
@weltenwort weltenwort changed the title [Logs UX] Classify modules sustainably [Logs UX] Classify logs-data-access and logs-shared as platform plugins Nov 22, 2024
@weltenwort weltenwort added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 10, 2024
@@ -74,7 +68,7 @@ export const LogEntryRow = memo(
scale,
wrap,
}: LogEntryRowProps) => {
const trackMetric = useUiTracker({ app: 'infra_logs' });
const trackMetric = useUiTracker();
Copy link
Member Author

Choose a reason for hiding this comment

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

The in-lined version of this is hard-coded to infra_logs.

@@ -20,8 +20,7 @@ import {
} from '../types';

export type PluginKibanaContextValue = CoreStart &
LogsSharedClientStartDeps &
LogsSharedClientStartExports;
LogsSharedClientStartDeps & { logsShared: LogsSharedClientStartExports };
Copy link
Member Author

Choose a reason for hiding this comment

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

This aligns the shape of the context with that of the other consuming plugins. This way no distinction of local and non-local usage is necessary at the usage site.

@weltenwort weltenwort marked this pull request as ready for review December 11, 2024 09:29
@weltenwort weltenwort requested review from a team as code owners December 11, 2024 09:29
@weltenwort weltenwort requested review from a team as code owners December 11, 2024 09:29
@elasticmachine
Copy link
Contributor

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

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.

Obs UX Infra changes LGTM / code review only

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Dec 11, 2024
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!)

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 review only LGTM, I see we'll get some duplicated utils between observability-shared and logs-shared, but it doesn't seem anything urgent to refactor yet 👌

@weltenwort
Copy link
Member Author

@tonyghiani thanks for the review!

I see we'll get some duplicated utils between observability-shared and logs-shared, but it doesn't seem anything urgent to refactor yet

Yes, that was the proposed short-term solution to cut the dependency ties in the issue. While there was an obvious shared package to move the APM locators to, I didn't want to add more complexity by creating some more packages for just a few lines.

@weltenwort
Copy link
Member Author

@elasticmachine merge upstream

@fkanout fkanout self-requested a review December 16, 2024 14:08
Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

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

Code review only LGTM.

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 16, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: dde3adc
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-201263-dde3adc4d932

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1922 1923 +1
dataQuality 90 91 +1
datasetQuality 277 278 +1
dataUsage 142 143 +1
discover 944 945 +1
infra 1542 1543 +1
inventory 257 258 +1
logsExplorer 569 570 +1
logsShared 376 379 +3
observability 1107 1108 +1
observabilityAIAssistantApp 414 415 +1
observabilityLogsExplorer 211 212 +1
observabilityOnboarding 255 256 +1
observabilityShared 221 231 +10
profiling 312 313 +1
unifiedDocViewer 216 217 +1
uptime 591 592 +1
ux 205 206 +1
total +29

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
@kbn/deeplinks-observability 53 58 +5
observabilityShared 516 518 +2
total +7

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 +1.0B
logsShared 294.6KB 291.0KB -3.6KB
total -3.6KB

Page load bundle

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

id before after diff
infra 57.8KB 57.8KB -79.0B
logsShared 174.8KB 174.8KB +6.0B
observabilityShared 93.7KB 93.8KB +37.0B
total -36.0B
Unknown metric groups

API count

id before after diff
@kbn/deeplinks-observability 65 70 +5
observabilityShared 522 524 +2
total +7

History

cc @weltenwort

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Core changes LGTM

Comment on lines +7 to +8
"group": "platform",
"visibility": "shared",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to move this out of the observability_solution directory to reflect this scoping in the dir structure too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine so, when this PR is merged, we will be able to re-run the relocation logic on the relocation PR.
Then, these modules will be placed under

x-pack/platform/plugins/shared/...

Correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

What remains to be seen, is if my script will remove the observability_solution fragment of the path with a transform function, I'll check to confirm.

@@ -2,29 +2,39 @@
"type": "plugin",
"id": "@kbn/logs-shared-plugin",
"owner": "@elastic/obs-ux-logs-team",
"group": "platform",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as for logs_data_access

Copy link
Contributor

@gsoldevila gsoldevila left a comment

Choose a reason for hiding this comment

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

LGTM on green! Since the ESLINT rule has been removed, CI will confirm whether the changes violate the no_group_crossing rules.

@weltenwort weltenwort merged commit d5e0d3a into elastic:main Dec 17, 2024
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12371445464

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 201263

Questions ?

Please refer to the Backport tool documentation

@weltenwort
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

weltenwort added a commit to weltenwort/kibana that referenced this pull request Dec 17, 2024
…ns (elastic#201263)

This classifies `@kbn/logs-data-access-plugin`,
`@kbn/logs-shared-plugin` and `@kbn/observability-logs-overview` as
"platform/shared".

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit d5e0d3a)

# Conflicts:
#	.eslintrc.js
weltenwort added a commit that referenced this pull request Dec 17, 2024
… plugins (#201263) (#204546)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Logs UX] Classify logs-data-access and logs-shared as platform
plugins (#201263)](#201263)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Felix
Stürmer","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-17T10:48:35Z","message":"[Logs
UX] Classify logs-data-access and logs-shared as platform plugins
(#201263)\n\nThis classifies
`@kbn/logs-data-access-plugin`,\r\n`@kbn/logs-shared-plugin` and
`@kbn/observability-logs-overview`
as\r\n\"platform/shared\".\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"d5e0d3a2f6cd7b9158950c7151d2aaba674214ca","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-logs"],"number":201263,"url":"https://github.com/elastic/kibana/pull/201263","mergeCommit":{"message":"[Logs
UX] Classify logs-data-access and logs-shared as platform plugins
(#201263)\n\nThis classifies
`@kbn/logs-data-access-plugin`,\r\n`@kbn/logs-shared-plugin` and
`@kbn/observability-logs-overview`
as\r\n\"platform/shared\".\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"d5e0d3a2f6cd7b9158950c7151d2aaba674214ca"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201263","number":201263,"mergeCommit":{"message":"[Logs
UX] Classify logs-data-access and logs-shared as platform plugins
(#201263)\n\nThis classifies
`@kbn/logs-data-access-plugin`,\r\n`@kbn/logs-shared-plugin` and
`@kbn/observability-logs-overview`
as\r\n\"platform/shared\".\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"d5e0d3a2f6cd7b9158950c7151d2aaba674214ca"}}]}]
BACKPORT-->
JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Dec 19, 2024
…ns (elastic#201263)

This classifies `@kbn/logs-data-access-plugin`,
`@kbn/logs-shared-plugin` and `@kbn/observability-logs-overview` as
"platform/shared".

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-logs Observability Logs User Experience Team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants