fix(inputs.mongodb): add an option to bypass connection errors on start

[papey]

Required for all PRs

• Updated associated README.md.
• Wrote appropriate unit tests.
• Pull request title or commits are in conventional commit format

resolves #10078

This PR replaces #10086.

Summary of changes :

• Added an option ignore_unreachable_hosts to not return an error on init if a ping fails

[Hipska]

My comments about moving the connect to mongo from the Init to the Start method still counts.

[papey]

My comments about moving the connect to mongo from the Init to the Start method still counts.

Oh yes, sorry. Will fix.

Edit: fixed

[papey]

Here is a refactor, I did not find an example of the Start function, so not sure if correct. If not correct, can you provide some example ? Thanks a lot for the feedback.

[Hipska]

This is the interface: input.go

And these are some sample implementations:

telegraf/plugins/inputs/sql/sql.go

Lines 354 to 392 in b07e94b

	func (s *SQL) Start(_ telegraf.Accumulator) error {
	var err error
	// Connect to the database server
	s.Log.Debugf("Connecting to %q...", s.Dsn)
	s.db, err = dbsql.Open(s.driverName, s.Dsn)
	if err != nil {
	return err
	}
	// Set the connection limits
	// s.db.SetConnMaxIdleTime(time.Duration(s.MaxIdleTime)) // Requires go >= 1.15
	s.db.SetConnMaxLifetime(time.Duration(s.MaxLifetime))
	s.db.SetMaxOpenConns(s.MaxOpenConnections)
	s.db.SetMaxIdleConns(s.MaxIdleConnections)
	// Test if the connection can be established
	s.Log.Debugf("Testing connectivity...")
	ctx, cancel := context.WithTimeout(context.Background(), time.Duration(s.Timeout))
	err = s.db.PingContext(ctx)
	cancel()
	if err != nil {
	return fmt.Errorf("connecting to database failed: %v", err)
	}
	// Prepare the statements
	for i, q := range s.Queries {
	s.Log.Debugf("Preparing statement %q...", q.Query)
	ctx, cancel := context.WithTimeout(context.Background(), time.Duration(s.Timeout))
	stmt, err := s.db.PrepareContext(ctx, q.Query) //nolint:sqlclosecheck // Closed in Stop()
	cancel()
	if err != nil {
	return fmt.Errorf("preparing query %q failed: %v", q.Query, err)
	}
	s.Queries[i].statement = stmt
	}
	return nil
	}

telegraf/plugins/inputs/mqtt_consumer/mqtt_consumer.go

Lines 151 to 166 in b07e94b

	func (m *MQTTConsumer) Start(acc telegraf.Accumulator) error {
	m.state = Disconnected
	m.acc = acc.WithTracking(m.MaxUndeliveredMessages)
	m.sem = make(semaphore, m.MaxUndeliveredMessages)
	m.ctx, m.cancel = context.WithCancel(context.Background())
	m.client = m.clientFactory(m.opts)
	// AddRoute sets up the function for handling messages. These need to be
	// added in case we find a persistent session containing subscriptions so we
	// know where to dispatch persisted and new messages to. In the alternate
	// case that we need to create the subscriptions these will be replaced.
	for _, topic := range m.Topics {
	m.client.AddRoute(topic, m.recvMessage)
	}
	m.state = Connecting
	return m.connect()
	}

[Hipska]

Looks good overall, but shouldn't the ping then also be done at the start of the gathering for the server, it will reduce the amount of errors every gather interval?

[papey]

Looks good overall, but shouldn't the ping then also be done at the start of the gathering for the server, it will reduce the amount of errors every gather interval?

Sounds good to me but how do we want that ?

In the current implementation on master if one of the Mongo servers becomes unreachable after init/start errors will be written in the logs.

IMO, it's ok to attach the behavior you describe to the new option, WDYT ?

[Hipska]

Yeah, so I was thinking, if this is enabled, then first ping the device and then only try to gather all the metrics if it does respond ok? Does that sounds reasonable?

[papey]

LGTM, i will implement it asap !

[papey]

There is just one behavior left we may want to discuss :

When Gather is called on an unreachable server it waits a very looong time before returning back an error. This is the current implementation and this PR do not change anything about it. We may want to change it in another PR. WDYT ?

Sorry for the the review requests mess, my internet was lagging and Github just goes crazy for no reason

[Hipska]

Do you mean this is the current default behaviour? I think that's good, they can change to retry when they don't want that. But when thinking about that, "retry" might give wrong impression. I would expect telegraf to retry in the same poll interval. Maybe the value for this option should be "skip"?

Also, I don't get the new logic in Gather when behaviour is "retry". So seems like it returns and skips collection for the server if there is no error? That sounds a bit strange.

[reimda]

When Gather is called on an unreachable server it waits a very looong time before returning back an error. This is the current implementation and this PR do not change anything about it. We may want to change it in another PR. WDYT ?

Sounds good to me to work on that in a future PR.

[papey]

@Hipska I changed the wording from "retry" to "skip", I also find it more clear. Thanks.

[papey]

Do you mean this is the current default behaviour? I think that's good, they can change to retry when they don't want that. But when thinking about that, "retry" might give wrong impression. I would expect telegraf to retry in the same poll interval. Maybe the value for this option should be "skip"?

Also, I don't get the new logic in Gather when behaviour is "retry". So seems like it returns and skips collection for the server if there is no error? That sounds a bit strange.

Sorry I didn't get your point, if behavior is skip, we ping, if ping fails, we drop fetch from the current server :

			if m.DisconnectedServersBehavior == "skip" {
				if err := srv.ping(); err != nil {
					return
				}
			}

[Hipska]

Sorry I didn't get your point, if behavior is skip, we ping, if ping fails, we drop fetch from the current server :

Nevermind, I was indeed looking wrong. Maybe we could add a debug log message with the content of the error before we return?

[telegraf-tiger]

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

🥳 This pull request decreases the Telegraf binary size by -1.04 % for linux amd64 (new size: 150.2 MB, nightly size 151.7 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz