[add_docker_metadata] log the error instead of returning it

[VihasMakwana]

This PR updates add_docker_metadata to log the error instead of returning it.

Relates elastic/elastic-agent#11171

[github-actions]

🤖 GitHub comments

Just comment with:

• run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @VihasMakwana? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
• backport-active-all is the label that automatically backports to all active branches.
• backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
• backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

[elasticmachine]

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b bump-elastic-autodiscover upstream/bump-elastic-autodiscover
git merge upstream/main
git push upstream bump-elastic-autodiscover

[cmacknz]

This should have a change entry

[cmacknz]

Looking closer at elastic/elastic-agent-autodiscover#145

It seems like the add_docker_metadata already attempts to treat not having docker as non-fatal, but it doesn't handle errors from Start the way it does from New and it should:

beats/libbeat/processors/add_docker_metadata/add_docker_metadata.go

Lines 88 to 98 in 55629bc

	watcher, err := watcherConstructor(log, config.Host, config.TLS, config.MatchShortID)
	if err != nil {
	dockerAvailable = false
	log.Debugf("%v: docker environment not detected: %+v", processorName, err)
	} else {
	dockerAvailable = true
	log.Debugf("%v: docker environment detected", processorName)
	if err = watcher.Start(); err != nil {
	return nil, fmt.Errorf("failed to start watcher: %w", err)
	}
	}

Elastic Agent does this properly for example

https://github.com/elastic/elastic-agent/blob/36a2197f33d0260f09be1b19437af995f57a6218/internal/pkg/composable/providers/docker/docker.go#L52-L56

I'm not sure we need elastic/elastic-agent-autodiscover#145 to fix this, we should continue returning an error, and then at the call site log+ignore it.

[cmacknz]

beats/libbeat/autodiscover/providers/docker/docker.go

Lines 112 to 114 in 55629bc

	if err := watcher.Start(); err != nil {
	return nil, errWrap(err)
	}

This change will make the beats docker auto-discovery provider continue even if it can't start the docker watcher, which will probably make it sit there silently not working.

This has to be explicitly configured from what I can tell so I'm not sure we want to change this globally, just fix add_docker_metadata's handling of Start errors:
https://www.elastic.co/docs/reference/beats/filebeat/configuration-autodiscover#_docker_2

[VihasMakwana]

@cmacknz Could you take a look when you're back?

[cmacknz]

A couple suggestions on wording but the change otherwise LGTM

Missing a test

[cmacknz]

Hit approve too early, we should add a test for this case. There is already a test for the no docker case before Start:

beats/libbeat/processors/add_docker_metadata/add_docker_metadata_test.go

Lines 100 to 113 in 55629bc

	func TestInitializationNoDocker(t *testing.T) {
	var testConfig = config.NewConfig()
	testConfig.SetString("host", -1, "unix:///var/run42/docker.sock")
	p, err := buildDockerMetadataProcessor(logp.L(), testConfig, docker.NewWatcher)
	assert.NoError(t, err, "initializing add_docker_metadata processor")
	input := mapstr.M{}
	result, err := p.Run(&beat.Event{Fields: input})
	assert.NoError(t, err, "processing an event")
	assert.Equal(t, mapstr.M{}, result.Fields)
	}

There is already a mock watcher in there which is probably what you have to modify to also test the start case.

[VihasMakwana]

@cmacknz Done!

[VihasMakwana]

CI is green on elastic-agent with this PR: elastic/elastic-agent#11374