Skip to content

Allow starting the beat without an output when CM is enabled #8567

Merged
exekias merged 3 commits intoelastic:masterfrom
exekias:cm-no-output
Oct 19, 2018
Merged

Allow starting the beat without an output when CM is enabled #8567
exekias merged 3 commits intoelastic:masterfrom
exekias:cm-no-output

Conversation

@exekias
Copy link
Copy Markdown
Contributor

@exekias exekias commented Oct 4, 2018

Central management will take care of configuring the output, so we
are allowed to start the beat with a nil output

@exekias exekias added review libbeat needs_backport PR is waiting to be backported to other branches. labels Oct 4, 2018
@exekias exekias requested a review from urso October 4, 2018 11:58
@exekias exekias changed the title oCombine fields.yml properties when they are defined in different so… Allow starting the beat without an output when CM is enabled Oct 4, 2018
@exekias exekias mentioned this pull request Oct 4, 2018
@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Oct 5, 2018

I'm a bit surprised about the CI failure as this should be fixed in master. Can you rebase this on top of the most recent version of master?

@exekias
Copy link
Copy Markdown
Contributor Author

exekias commented Oct 5, 2018

rebased, let's see if it passes now

Copy link
Copy Markdown
Member

@jsoriano jsoriano Oct 16, 2018

Choose a reason for hiding this comment

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

We could keep logging something, in case someone configures beats using CM, but still without an output.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1 for some debug/info log here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your input folks, pushed 34bba0d7f, I struggled a bit with the messaging here, I'm open for feedback on it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd add just an info line mentioning that current loaded configuration has no outputs and then no data will be published. So if after configuring modules someone find that they still don't publish anything, they can find a clue to their mistake in the logs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the current message: No outputs are configured, starting in paused mode., wdyt?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like you added the Info log to the wrong line.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Umm, you added it to the dry-run flow, was this intended?

Regarding the message, I was more concerned to the case of the user changing the config using CM but not adding any output (or removing this by mistake if possible), so not only when the beat starts, also I wouldn't talk about a "paused mode" (do we have this concept?).
What about something that announces the effect to the user, like No outputs are configured, no data will be collected?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I want to avoid something that will make the user think this is bad, as it will be there if you are using central management. Best message to me would be something like this:

Output settings are centrally managed, go to Kibana to configure one

Now, I understand we don't want to be that specific from the outRequired flag, so I'm seeking for a message that explains that as soon as you see CM logs after it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Output configuration pending?

Carlos Pérez-Aradros Herce added 2 commits October 17, 2018 17:42
Central management will take care of configuring the output, so we
are allowed to start the beat with a nil output
}

if publishDisabled {
log.Info("No outputs are configured, starting in paused mode.")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This condition is not about 'outRequired', but we end here if publishing is disabled from the CLI, even if outputs are configured.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ I'll move it 😇 are we ok with the text istelf?

@exekias
Copy link
Copy Markdown
Contributor Author

exekias commented Oct 19, 2018

as discussed offline, we decided to move the error message up in the stack so it can be more meaninful, please have a look :)

@exekias
Copy link
Copy Markdown
Contributor Author

exekias commented Oct 19, 2018

Failing test is unrelated, addressing it here: #8656

@exekias exekias merged commit 1012302 into elastic:master Oct 19, 2018
@exekias exekias added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Oct 19, 2018
exekias added a commit to exekias/beats that referenced this pull request Oct 22, 2018
…#8567)

* Allow starting the beat without an output when CM is enabled

Central management will take care of configuring the output, so we
are allowed to start the beat with a nil output

(cherry picked from commit 1012302)
exekias added a commit that referenced this pull request Oct 22, 2018
…8657)

* Allow starting the beat without an output when CM is enabled

Central management will take care of configuring the output, so we
are allowed to start the beat with a nil output

(cherry picked from commit 1012302)
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.

4 participants