-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add execution context support for management plugins #128415
Add execution context support for management plugins #128415
Conversation
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
src/core/public/mocks.ts
Outdated
import { executionContextServiceMock } from './execution_context/execution_context_service.mock'; | ||
export { executionContextServiceMock } from './execution_context/execution_context_service.mock'; |
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.
Outside of the scope of the PR, but would you mind moving the import
statement with the others a few lines up 🙏 ?
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.
👍 done
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.
Thanks for working on this @alisonelizabeth, that's a lot of files! Changes lgtm, left a just one suggestion for a snapshot and restore label that looks a bit funny, but no blockers
import { EuiPageContent, EuiPageBody, EuiEmptyPrompt } from '@elastic/eui'; | ||
|
||
export class App extends Component { |
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.
🤘🏼
|
||
// Track component loaded | ||
useEffect(() => { | ||
uiMetricService.trackUiMetric(UIM_RESTORE_LIST_LOAD); | ||
}, [uiMetricService]); | ||
|
||
useExecutionContext(core.executionContext, { | ||
type: 'application', | ||
page: 'snapshotRestoreRestores', |
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 one reads a bit funny, what do you think about snapshotRestoreList
or snapshotRestoreRestoreList
🤔
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.
What do you think about snapshotRestoreRestoreTab
? I think I originally had snapshotRestoreRestoreList
, but changed it since it actually covers any requests within the tab (not just the list view).
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.
Yeah I think snapshotRestoreRestoreTab
sounds a lot better!
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled in files
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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 is excellent - Thank you!
Once merged, all date in APM and Fullstory should be labeled with these page names - I'm sure this will really help understanding the workflows!
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Fixes #126802
This PR adds execution context support to the platform deployment management apps.
Dev Tools (Search Profiler, Console, Painless Lab, Grokdebugger)Implemented via APM execution context - app, page, entitiy id #124996Upgrade AssistantNot applicable, as UA is only enabled in 7.17 and this feature is available in 8.2+Note: I only added this at the top-level app render, with the exception of Index Management and Snapshot + Restore which has it per tab. If we want, we can be more granular and register each route. I propose the current approach, and modify as needed once we start to analyze the data.
How to test
Follow the "How to test this PR?" instructions in #124996 (comment).
Navigate to the list of apps above and verify the events have the correct labels.