[User Experience App] Move UX public code out of apm #88645
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💔 Build Failed
Failed CI StepsTest FailuresJest Tests.x-pack/plugins/apm/public/components/app/service_inventory.ServiceInventory should render services, when list is not emptyStandard OutStack TraceJest Tests.x-pack/plugins/apm/public/components/app/service_inventory.ServiceInventory should render empty message, when list is empty and historical data is foundStandard OutStack TraceJest Tests.x-pack/plugins/apm/public/components/app/service_inventory.ServiceInventory when legacy data is found renders an upgrade migration notificationStandard OutStack Traceand 12 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: |
shahzad31
changed the title
shahzad31
added
v8.2.0
release_note:skip
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled in files
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @shahzad31 |
justinkambic
approved these changes
Feb 3, 2022
| @@ -0,0 +1 @@ | |||
| ## Observability User Experience Application | |||
There was a problem hiding this comment.
Might be cool to add some copy here vs. having an empty file.
|
Pinging @elastic/apm-ui (Team:apm) |
jbudz
approved these changes
Feb 3, 2022
dgieselaar
approved these changes
Feb 4, 2022
There was a problem hiding this comment.
Curious if we can remove some other stuff to the obs plugin or a separate package, but other than that it is looking good, thanks a million @shahzad31 !
| @@ -121,3 +121,4 @@ pageLoadAssetSize: | |||
| controls: 34788 | |||
| expressionPie: 26338 | |||
| sharedUX: 16225 | |||
| ux: 20784 | |||
There was a problem hiding this comment.
do you mind updating the one for APM?
There was a problem hiding this comment.
hmm shouldn't this happen auto? i ran the script
| @@ -118,3 +118,9 @@ export const uxLocalUIFilters = uxLocalUIFilterNames.reduce((acc, key) => { | |||
| }, | |||
| }; | |||
| }, {} as UxLocalUIFilterMap); | |||
|
|
|||
| export type UxUIFilters = { | |||
There was a problem hiding this comment.
any chance we can move this entire file?
There was a problem hiding this comment.
this one is tricky, it's being used in APM server. And UX plugin is using some stuff from APM. So for now relationship is one directional. UX app is dependant on APM. I kept it here for now , to not introduce circular dependency.
Once routes are removed from APM, this will be gone auto.
| @@ -8,7 +8,13 @@ | |||
| import { getRumPageLoadTransactionsProjection } from '../../projections/rum_page_load_transactions'; | |||
| import { mergeProjection } from '../../projections/util/merge_projection'; | |||
| import { SetupUX } from './route'; | |||
| import { BreakdownItem } from '../../../typings/ui_filters'; | |||
|
|
|||
| export interface BreakdownItem { | |||
There was a problem hiding this comment.
is it possible to move this entire folder (rum_client/) out of the apm plugin?
There was a problem hiding this comment.
Yes, this will be a follow up.
| @@ -0,0 +1,20 @@ | |||
| /* | |||
There was a problem hiding this comment.
any chance we can move this file to observability or keep it in APM and have the UX app import it?
| @@ -0,0 +1,42 @@ | |||
| /* | |||
There was a problem hiding this comment.
there is already a copy in the obs plugin, so this will be the 3rd one - any chance we can consolidate? maybe as a separate package?
There was a problem hiding this comment.
definitely, a follow up. i think we will move this to observability plugin. i am not sold on moving it to a package.
kibanamachine
added
the
backport missing
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
shahzad31
added
backport:skip
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #78430
Moves existing UX dashboard code from APM plugin to new ui User Experience plugin.
This is the first step in the refactoring, more follow ups should be expected to untangle more things.
Test cases
Make sure you do these test cases before approving the PR
We will add/enable e2e tests in a follow up PR