-
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
[Uptime] Uptime to APM integration #34892
[Uptime] Uptime to APM integration #34892
Conversation
Pinging @elastic/uptime |
💔 Build Failed |
5e5c4bc
to
b9c94ac
Compare
💔 Build Failed |
b9c94ac
to
0e34a0f
Compare
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
x-pack/plugins/uptime/public/components/functional/monitor_list_actions_popover.tsx
Show resolved
Hide resolved
} | ||
)} | ||
color="subdued" | ||
iconType="boxesHorizontal" |
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.
Maybe there's a better icon option now that we don't show other icons next to this. The boxes make sense as an ellipsis to other icons, but alone they are weird. See the SS below:
There is a popout icon, but that's not really right for a popover:
Maybe we could get some input from @elastic/eui-design or @elastic/kibana-design ?
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.
@andrewvc I am fine with changing the icon. Would you be ok with me making a follow-up issue where we could discuss this in more detail so we don't hold up this PR any longer? There are other PRs downstream of this one that are blocked until some of this code gets merged.
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.
Yes, I'm OK with that
Code and functionality LGTM (WFG). Two follow-ups:
If this winds up being merged before those things are done, can you create issues for those two things @justinkambic ? |
jenkins test this |
Sure thing. The first one will almost definitely be an issue, the second one won't because I'm not planning to merge this before that PR. |
💔 Build Failed |
💚 Build Succeeded |
* Add integrations popover. * Add some more functionality, code is WIP/mocked. * Trying some things WIP. * Import settings values from context. * Remove obsolete comment. * Add links. * Rename component. Clean up placeholder text and add translations. * Minor tweaks. Rename component file. * Fix import for renamed file. * Add domain to api query result fixtures. * Change integration to utilize EuiTable's actions API. * Add translation for new column heading. * Update busted snapshot. * Add snapshot test for new component. * Refactor integration links to dedicated component. * Remove obsolete index export. * Update monitor list test snapshot. * Default monitor list to empty array instead of undefined. * Extract URL construction to helper function. * Make entire link text clickable for APM integration. * Update broken test snapshot. * Fix type and update test snapshot.
* Add integrations popover. * Add some more functionality, code is WIP/mocked. * Trying some things WIP. * Import settings values from context. * Remove obsolete comment. * Add links. * Rename component. Clean up placeholder text and add translations. * Minor tweaks. Rename component file. * Fix import for renamed file. * Add domain to api query result fixtures. * Change integration to utilize EuiTable's actions API. * Add translation for new column heading. * Update busted snapshot. * Add snapshot test for new component. * Refactor integration links to dedicated component. * Remove obsolete index export. * Update monitor list test snapshot. * Default monitor list to empty array instead of undefined. * Extract URL construction to helper function. * Make entire link text clickable for APM integration. * Update broken test snapshot. * Fix type and update test snapshot.
Summary
Add APM integration. The intended result of these changes is to allow users to quickly and efficiently navigate from Uptime to APM, with a pre-filtered list of services based on the domain of a selected service/host they're monitoring in both solutions.
Example: Jennifer is monitoring her company's production site home page and notices several blips in its sparkline on the Uptime monitor list. She clicks the APM integration button and is navigated to the APM app, where she can view internal app performance and determine if there are actionable problems related to the 404s she noticed.
WIP.Testing
Prereq's
To test these changes, we will need to do some configuration. The prerequisites to testing this feature is to have:
apm-*
patternA few options for configuring APM/your service:
Testing functionality
After you've configured your APM server/service, configure Heartbeat to monitor the same service. Then load the Uptime UI and attempt to follow the integration link:
url.domain
field should correspond to the domain that Heartbeat has supplied.Visual review
After conferring with the design team I've decided that it's better to implement these integrations as Actions Column items using the API provided by
EuiBasicTable
.The primary thing we are adding here is a popover to our Monitors list component.We have an icon button to expose the popover:Today we are just adding an integration for APM. We will be adding additional integrations to this popover targeting 7.1. The expanded popover looks like:Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers