Add support for heartbeat hints builder #13119

vjsamuel · 2019-07-31T08:55:11Z

This PR allows hints builder to pick up heartbeat configs from hints like kubernetes annotations and docker labels.

The syntax would be as follows:

co.elastic.heartbeats/type: icmp
co.elastic.heartbeats/hosts: ${data.host}
co.elastic.heartbeats/schedule: "@every 5s"

Heartbeat seems to be a unique use case as a user might want more than one check on his/her pod/container. So i have added the following:

co.elastic.heartbeats/1.type: tcp
co.elastic.heartbeats/1.hosts: ${data.host}:9090
co.elastic.heartbeats/1.schedule: "@every 5s"

co.elastic.heartbeats/1.type: tcp
co.elastic.heartbeats/2.hosts: ${data.host}:9091
co.elastic.heartbeats/2.schedule: "@every 5s"

elasticmachine · 2019-07-31T08:55:13Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2019-07-31T08:55:13Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

houndci-bot · 2019-07-31T08:55:14Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+	logger *logp.Logger
+}
+
+// NewLogHints builds a log hints builder


comment on exported function NewHeartbeatHints should be of the form "NewHeartbeatHints ..."

elasticmachine · 2019-07-31T09:11:02Z

Pinging @elastic/uptime

exekias · 2019-07-31T09:34:34Z

This is great @vjsamuel! I will dig more into the code, at first sight I wonder if we should use co.elastic.monitor instead of heartbeats. As we advance in agents unification I think that will be a better suited name. Like we did with Filebeat & Metricbeat (logs & metrics).

cc @andrewvc FYI

andrewvc · 2019-08-10T01:53:07Z

Apologies for the delay, but this is something I'll look into when back from vacation in a few weeks!

andrewvc

Thanks for the awesome work and we're looking forward to getting this merged! This is a partial review. I haven't actually run the code yet, and would like to take a second pass.

I left some comments with mostly some minor naming concerns. I do also have some concerns about error handling I left in there. Generally, it's best to avoid any situation where we just skip doing something due to a config error without a good diagnostic log message IMHO. Would be glad to hear your thoughts on that.

andrewvc · 2019-08-31T14:56:37Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+
+	// If explicty disabled, return nothing
+	if builder.IsDisabled(hints, l.config.Key) {
+		l.logger.Debugf("heartbeats disabled by hint: %+v", event)


`s/heartbeats/heartbeat config/

andrewvc · 2019-08-31T14:59:02Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+	for _, monitor := range monitors {
+		// Every monitor must have a type associated with it
+		if _, ok := monitor[montype]; !ok {
+			continue


It might be good to log a warning here. Otherwise, debugging config issues could be hard.

i have made it a debug message

Why debug? I was thinking we'd log at warn or info at the very least. Would that be too verbose for that use case that precipitated this PR?

andrewvc · 2019-08-31T15:00:35Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+}
+
+// Create config based on input hints in the bus event
+func (l *heartBeat) CreateConfig(event bus.Event) []*common.Config {


I'm curious as to the choice of the variable name l for *heartBeat. Why not just hb or h.

lol, i took the logs hints builder as a base and worked on this. copy paste

andrewvc · 2019-08-31T15:01:45Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+	processors = "processors"
+)
+
+type heartBeat struct {


given that we have a public Heartbeat struct in heartbeat.go this is a bit confusing. Maybe we could find a better name here. One idea is hintsContext.

andrewvc · 2019-08-31T15:23:55Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+// specific language governing permissions and limitations
+// under the License.
+
+package hints


It feels odd to name this file heartbeats.go, That implies that it has some plural relationship to heartbeat. Maybe hints.go?

i just followed the convention for logs and metrics hints builder in filebeat and metricbeat.

Ah, I see. Looking at metric beat it named its file metrics. Given that I think the right name here is monitors.go. It seems the convention is to name the file the same as the noun of the thing it is generating, which in heartbeat's case is a monitor.

heartbeat/autodiscover/builder/hints/heartbeats.go

andrewvc · 2019-08-31T15:28:33Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+		} else if port == 0 && !strings.Contains(h, ":") {
+			// For ICMP like use cases allow only host to be passed if there is no port
+			result = append(result, h)
+		}


What do you think about adding a default else case here letting the user know their config won't run? In general, it's good to avoid silent failures since they're hard to debug.

the reason why this is not defaulted to just else is because we dont want to create a host config unless we can make use of it meaningfully. in such scenarios we wont spin up a monitor because it fits neither host nor host-port patterns.

I think there was a bit of a misunderstanding here. I was saying that we should probably log an error in the else case.

andrewvc · 2019-08-31T15:30:19Z

heartbeat/autodiscover/builder/hints/heartbeats_test.go

+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/elastic/beats/libbeat/logp"


imports need some cleanup, they should be grouped

andrewvc · 2019-08-31T15:33:55Z

heartbeat/autodiscover/builder/hints/heartbeats_test.go

+			result: common.MapStr{},
+		},
+		{
+			message: "Hints without host should return nothing",


Going back to earlier comments, if there are some heartbeat hints required, but not others, we should log an error. It'd be great to check for that log message in our tests. It might be a bit challenging with our current code structure. One thought is that the builder could return warnings (maybe just as a []string) in addition to errors. That'd be easy to test.

i have added debug messages to the hint generation code like how the other hints based builders do for filebeat and metricbeat. we can discuss this further when @exekias is back and do it uniformly across all 3 if it is ok

andrewvc · 2019-08-31T15:37:00Z

jenkins, retest this please

vjsamuel · 2019-09-16T18:08:38Z

the build should pass now. incorporated most review comments and commented on the rest. please take a look @andrewvc

andrewvc

Great work, this is really close to being merged. I left some comments about the log levels. It's really important we get this right, so that when users make mistakes they can see the issues in their logs and troubleshoot them.

Thanks once again!

andrewvc · 2019-09-20T17:58:01Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+// specific language governing permissions and limitations
+// under the License.
+
+package hints


Ah, I see. Looking at metric beat it named its file metrics. Given that I think the right name here is monitors.go. It seems the convention is to name the file the same as the noun of the thing it is generating, which in heartbeat's case is a monitor.

andrewvc · 2019-09-20T18:00:26Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+	for _, monitor := range monitors {
+		// Every monitor must have a type associated with it
+		if _, ok := monitor[montype]; !ok {
+			continue


Why debug? I was thinking we'd log at warn or info at the very least. Would that be too verbose for that use case that precipitated this PR?

andrewvc · 2019-09-20T18:02:15Z

heartbeat/autodiscover/builder/hints/heartbeats.go

+		} else if port == 0 && !strings.Contains(h, ":") {
+			// For ICMP like use cases allow only host to be passed if there is no port
+			result = append(result, h)
+		}


I think there was a bit of a misunderstanding here. I was saying that we should probably log an error in the else case.

andrewvc

This is almost ready to merge, but looking at it one last time I'm realizing my prior comments didn't make sense. We should show config errors at a higher level, we're showing them too early here. https://github.com/elastic/beats/pull/13675/files should show an easy to debug message with sensitive variables censored.

Once those two checks are merged, and tests are passing, I can merge.

andrewvc · 2019-09-23T18:49:01Z

heartbeat/autodiscover/builder/hints/monitors.go

+
+		h, ok := hb.getHostsWithPort(monitor, port)
+		if !ok {
+			hb.logger.Warn("hosts to monitor is mandatory")


Let's delete this whole check too.

andrewvc · 2019-09-23T18:49:23Z

heartbeat/autodiscover/builder/hints/monitors.go

+	for _, monitor := range monitors {
+		// Every monitor must have a type associated with it
+		if _, ok := monitor[montype]; !ok {
+			hb.logger.Warn("type of monitor is mandatory")


I don't think we actually need this check here, this should be caught at a higher level where we can also add more context. https://github.com/elastic/beats/pull/13675/files should improve things.

andrewvc · 2019-09-23T19:28:53Z

jenkins, test this please

andrewvc

LGTM WFG

andrewvc · 2019-09-23T22:25:39Z

Build failures unrelated

Autodiscover hints were introduced in elastic#13119 but not documented. This adds the docs. Resolves elastic#14245

Autodiscover hints were introduced in #13119 but not documented. This adds the docs. Resolves #14245

Autodiscover hints were introduced in elastic#13119 but not documented. This adds the docs. Resolves elastic#14245 (cherry picked from commit 4eb1711)

Autodiscover hints were introduced in #13119 but not documented. This adds the docs. Resolves #14245

* [Uptime] Add docs for autodiscover hints (#14252) Autodiscover hints were introduced in #13119 but not documented. This adds the docs. Resolves #14245 (cherry picked from commit 4eb1711) * Fix double include

Autodiscover hints were introduced in elastic#13119 but not documented. This adds the docs. Resolves elastic#14245

vjsamuel requested review from a team as code owners July 31, 2019 08:55

houndci-bot reviewed Jul 31, 2019

View reviewed changes

exekias added Team:obs-ds-hosted-services Label for the Observability Hosted Services team containers Related to containers use case enhancement Heartbeat labels Jul 31, 2019

vjsamuel force-pushed the heartbeat_hints branch from 06db8ad to 02939f9 Compare July 31, 2019 18:49

andrewvc reviewed Aug 31, 2019

View reviewed changes

vjsamuel force-pushed the heartbeat_hints branch 7 times, most recently from 712594f to af58a1d Compare September 16, 2019 18:06

vjsamuel force-pushed the heartbeat_hints branch from af58a1d to 9ef142b Compare September 17, 2019 21:20

andrewvc added the v7.5.0 label Sep 19, 2019

andrewvc requested changes Sep 20, 2019

View reviewed changes

vjsamuel force-pushed the heartbeat_hints branch from 9ef142b to a143932 Compare September 23, 2019 18:42

andrewvc requested changes Sep 23, 2019

View reviewed changes

vjsamuel force-pushed the heartbeat_hints branch 2 times, most recently from b0f3c42 to b77d748 Compare September 23, 2019 19:16

Add support for heartbeat hints builder

7971846

vjsamuel force-pushed the heartbeat_hints branch from b77d748 to 7971846 Compare September 23, 2019 19:17

andrewvc approved these changes Sep 23, 2019

View reviewed changes

andrewvc merged commit 3f249f3 into elastic:master Sep 23, 2019

urso added the release-highlight label Oct 22, 2019

This was referenced Oct 25, 2019

[Heartbeat] Add docs for hints based autodiscover #14245

Closed

Support Autodiscover hints in heartbeat #8552

Closed

andrewvc added a commit to andrewvc/beats that referenced this pull request Oct 25, 2019

[Uptime] Add docs for autodiscover hints

b20b3d2

Autodiscover hints were introduced in elastic#13119 but not documented. This adds the docs. Resolves elastic#14245

andrewvc mentioned this pull request Oct 25, 2019

[Uptime] Add docs for autodiscover hints #14252

Merged

andrewvc added a commit that referenced this pull request Nov 13, 2019

[Uptime] Add docs for autodiscover hints (#14252)

4eb1711

Autodiscover hints were introduced in #13119 but not documented. This adds the docs. Resolves #14245

andrewvc mentioned this pull request Nov 19, 2019

[Uptime] Add docs for autodiscover hints (#14252) #14622

Merged

andrewvc mentioned this pull request Nov 19, 2019

[Uptime] Add docs for autodiscover hints (#14252) #14623

Merged

andrewvc added a commit that referenced this pull request Nov 25, 2019

[Uptime] Add docs for autodiscover hints (#14252) (#14623)

ec59f05

Autodiscover hints were introduced in #13119 but not documented. This adds the docs. Resolves #14245

vjsamuel mentioned this pull request Jun 17, 2020

Add support for multiple sets of hints on autodiscover #18883

Merged

6 tasks

jsoriano mentioned this pull request Jun 23, 2020

Cherry-pick #18883 to 7.x: Add support for multiple sets of hints on autodiscover #19330

Merged

6 tasks

thenickfish mentioned this pull request Mar 29, 2021

[Heartbeat] Hints based autodiscover ports settings #17771

Closed

leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023

[Uptime] Add docs for autodiscover hints (elastic#14252) (elastic#14623)

c5eb329

Autodiscover hints were introduced in elastic#13119 but not documented. This adds the docs. Resolves elastic#14245


		"github.com/stretchr/testify/assert"

		"github.com/elastic/beats/libbeat/logp"

Add support for heartbeat hints builder #13119

Add support for heartbeat hints builder #13119

Conversation

vjsamuel commented Jul 31, 2019

elasticmachine commented Jul 31, 2019

elasticmachine commented Jul 31, 2019

Choose a reason for hiding this comment

elasticmachine commented Jul 31, 2019

exekias commented Jul 31, 2019

andrewvc commented Aug 10, 2019

andrewvc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Aug 31, 2019

vjsamuel commented Sep 16, 2019

andrewvc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Sep 23, 2019

andrewvc left a comment

Choose a reason for hiding this comment

andrewvc commented Sep 23, 2019