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

Central management for hb #9254

Merged
merged 2 commits into from
Dec 4, 2018
Merged

Central management for hb #9254

merged 2 commits into from
Dec 4, 2018

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Nov 27, 2018

Adds central management to heartbeat. Works together with elastic/kibana#26219

@andrewvc andrewvc added in progress Pull request is currently in progress. Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Nov 27, 2018
@andrewvc andrewvc added review needs_backport PR is waiting to be backported to other branches. and removed in progress Pull request is currently in progress. labels Nov 27, 2018
@andrewvc andrewvc requested a review from ruflin November 27, 2018 21:03
This patch adds support for CM features and documents them as well.
@@ -82,6 +84,10 @@ func (bt *Heartbeat) Run(b *beat.Beat) error {
return err
}

if b.ConfigManager.Enabled() {
bt.RunCentralMgmtMonitors(b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not registering these always? I'm wondering if there could be a timing issue here, something like:

  • Central Management fetches configs and calls reload, heartbeat.monitors is not yet registered
  • This code registers the reloadable after that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I just copied from https://github.com/elastic/beats/blob/master/metricbeat/beater/metricbeat.go#L109 but if it isn't necessary I'm glad to remove the conditional.

Is that safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked how it's done in Metricbeat and it could have the same issue, I'm guessing that even in the improbable case of this happening, it would be fixed by the next pass on config apply. 👍 to leave it like this

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Overall LGTM but see comment from @exekias

Let's make sure we test this manually as soon as KB PR is also merged.

@andrewvc
Copy link
Contributor Author

@exekias @ruflin I've added another commit that removes the CM docs from this patch. Thinking about it, Heartbeat will be administered from a dedicated Uptime UI, not CM, so those docs don't apply. Documenting this is premature. For now, I believe, we should leave the UI experience undocumented till we're further along.

Perhaps we should only merge this into master now until we have a story there.

@ruflin ruflin added Team:obs-ds-hosted-services Label for the Observability Hosted Services team and removed Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Dec 3, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime

@andrewvc andrewvc merged commit 79a6ff3 into elastic:master Dec 4, 2018
@andrewvc andrewvc removed the needs_backport PR is waiting to be backported to other branches. label Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Heartbeat review Team:obs-ds-hosted-services Label for the Observability Hosted Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants