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

[Infra UI] Adding links from Infra UI to Uptime #35993

Merged
merged 10 commits into from May 10, 2019

Conversation

simianhacker
Copy link
Member

@simianhacker simianhacker commented May 2, 2019

Summary

This PR closes #35294 by adding the a link to the node context menu. In support of the URL, this PR also adds the available IP address to the node path to be used in uptime monitoring.

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/infrastructure-ui

@simianhacker simianhacker changed the title [Infra UI] Adding from Infra UI to Uptime [Infra UI] Adding links from Infra UI to Uptime May 2, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker
Copy link
Member Author

@dov0211 Looks like host.ip is not shipped by default. I have the link setup to try and use host.ip when available and fallback to host.name if it's not.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@skh
Copy link
Contributor

skh commented May 8, 2019

Linking with host.ip works, but only if the following is added to the configuration of both heartbeat and metricbeat:

processors:
  - add_host_metadata:
      netinfo.enabled: true

I know we have already discussed this, but maybe we should explicitly track this in a separate issue to make sure it is documented somewhere. @dov0211, what do you think?

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

Great job, and really nice to see more tests.

Just one comment inline about how the IP addresses come back, but I'm not sure how likely this is to be a problem.

): string | null => {
const ip = get(bucket, `ip.hits.hits[0]._source.${IP_FIELDS[options.nodeType]}`);
// Use the first ip since it's the IPv4 IP address
return Array.isArray(ip) ? first(ip) : ip;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be sure that the IPv4 ip is always the first entry?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point, I think I will make a filter that matches the format to make sure it's the right one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this so it only returns the first IPv4 address it finds. I also added a unit test to make sure it works correctly 👍

@simianhacker simianhacker requested a review from skh May 8, 2019 17:01
@simianhacker
Copy link
Member Author

@skh I added the changes for the IPv4 stuff and re-submitted it for review. I think you could just look at the new commit: 61f5257

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@skh
Copy link
Contributor

skh commented May 9, 2019

@simianhacker Thank you for the changes and the additional tests 👍

There's one more issue though: heartbeat data shouldn't have the host.name field so we can't use it as a fallback. See elastic/beats#12107

@simianhacker
Copy link
Member Author

@dov0211 @justinkambic After some discussion it was agreed upon that we should only show the link for host nodes if the host.ip field exists. Otherwise it would fallback to searching using the host.name field which is less than ideal since host.name means something different in Uptime.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

👍

@simianhacker simianhacker merged commit 69bbca4 into elastic:master May 10, 2019
simianhacker added a commit to simianhacker/kibana that referenced this pull request May 10, 2019
* [Infra UI] Adding from Infra UI to Uptime

* Removed unused variable

* Adding tests for link generation

* Sometimes the IP address will be an array with [IPv4, IPv6]

* Ensuring only IPv4 addresses are returned; adding tests;

* only showing uptime link for host's with ip addresses
simianhacker added a commit that referenced this pull request May 17, 2019
* [Infra UI] Adding from Infra UI to Uptime

* Removed unused variable

* Adding tests for link generation

* Sometimes the IP address will be an array with [IPv4, IPv6]

* Ensuring only IPv4 addresses are returned; adding tests;

* only showing uptime link for host's with ip addresses
@simianhacker simianhacker deleted the link-to-uptime branch April 17, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infra UI] Cross-linking to Uptime
4 participants