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

Moved samara config out of init into connect #9051

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

helenosheaa
Copy link
Member

This is a fix for if Kafka doesn't connect it wont stop telegraf from starting up

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

🤝 ✅ CLA has been signed. Thank you!

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Mar 25, 2021
Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

@helenosheaa helenosheaa merged commit 5653362 into master Mar 25, 2021
@helenosheaa helenosheaa deleted the kafka-output-startup branch March 25, 2021 22:06
@Hipska
Copy link
Contributor

Hipska commented Mar 26, 2021

This only fixes the case for Kafka, but what about the other plugins that have this behaviour? #3723

@Hipska Hipska added area/kafka plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Mar 26, 2021
@helenosheaa
Copy link
Member Author

Thanks @Hipska, the team will revisit that open issue next week and add thoughts/plans to the issue

jblesener pushed a commit to jblesener/telegraf that referenced this pull request Apr 18, 2021
@ssoroka
Copy link
Contributor

ssoroka commented Jun 1, 2021

If anyone reading this later is confused, the point of this change is to make it so that Telegraf will not fail to start when this single output has a problem connecting.

To @Hipska 's question, this needs to be addressed on a per-plugin basis. Plugins should use the Init() (capital letter I) function to check config and error on a configuration problem that should prevent Telegraf from starting, and the Connect()/Start() functions for actually connecting to external services; if those return errors they will not prevent Telegraf from starting. Most plugins follow this pattern but a few got it wrong and went unnoticed.

@oplehto
Copy link
Contributor

oplehto commented Jun 2, 2021

To @Hipska 's question, this needs to be addressed on a per-plugin basis. Plugins should use the Init() (capital letter I) function to check config and error on a configuration problem that should prevent Telegraf from starting, and the Connect()/Start() functions for actually connecting to external services; if those return errors they will not prevent Telegraf from starting. Most plugins follow this pattern but a few got it wrong and went unnoticed.

100% this ^ Maybe the PR template should even have some checklist item of verifying what are the fatal/non-fatal failure modes and do they conform to the recommended pattern.

@Hipska
Copy link
Contributor

Hipska commented Jul 2, 2021

@ssoroka That would be very nice if it is actually true, but this code proves otherwise:

telegraf/agent/agent.go

Lines 253 to 257 in 17e86ab

err := si.Start(acc)
if err != nil {
stopServiceInputs(unit.inputs)
return nil, fmt.Errorf("starting input %s: %w", input.LogName(), err)
}

When the Start() returns an error, it will stop all other plugins and then exit..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kafka fix pr to fix corresponding bug plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants