[dockerstatsreceiver] add new log receiver to ingest docker events #36284

spiffyy99 · 2024-11-08T22:45:29Z

Description

Adds a log receiver for dockerstatsreceiver to ingest docker events. Adheres to the otel events standards

Receiver calls client.Events to return channel and polls from channel. Retries connection to docker daemon upon error with exponential backoff by calling Events method repeatedly (per client spec)

Link to tracking issue

Fixes 29096

Testing

Unit testing, e2e (in progress/todo)

Documentation

Modified readme.md with new config fields, in development status for log receiver, and description of receiver behavior.

spiffyy99 · 2024-11-14T04:58:21Z

Ready for review for the most part, just need to update integration tests.

spiffyy99 · 2024-11-23T06:31:03Z

@jamesmoessis could you take a look at this?

github-actions · 2024-12-08T05:20:52Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

jamesmoessis · 2024-12-09T07:10:09Z

@spiffyy99 apologies for the delay, I will review this tomorrow.

jamesmoessis · 2024-12-09T07:13:13Z

receiver/dockerstatsreceiver/factory.go

@@ -35,9 +36,22 @@ func createDefaultConfig() component.Config {
 			Timeout:          scs.Timeout,
 		},
 		MetricsBuilderConfig: metadata.DefaultMetricsBuilderConfig(),
+		MinDockerRetryWait:   time.Second,
+		MaxDockerRetryWait:   10 * time.Second,
+		Logs:                 EventsConfig{},


Will this collect events automatically? I think it should be off-by-default as to not affect existing deployments with the receiver running

Or will the log not be active unless they put it into a log pipeline... Good to know either way

the latter, i'd assume. because pipelines are created in the config like this:

pipelines: logs: receivers: ... exporters: ... processors: ...

so there'd be no reason to create a logs receiver unless it was used in a pipeline, even if the config called for one.

jamesmoessis

It looks pretty good to me. Just some minor comments and questions.

Given I am not too familiar with logs in the Collector, would be nice to have a review from a maintainer that knows logs well.

receiver/dockerstatsreceiver/README.md

jamesmoessis · 2024-12-10T06:09:09Z

receiver/dockerstatsreceiver/README.md


 - `endpoint` (default = `unix:///var/run/docker.sock`): Address to reach the desired Docker daemon.
+- `excluded_images` (no default, all running containers monitored): A list of strings,


Is there a scenario where you want to collect logs from one set of images, and metrics from another set? Currently it's set to both. I think, in this scenario you probably just create two different configs, but nevertheless thought I'd bring it up.

yeah what can be done in this case is creating a different receiver entirely. i'll mention this case in the documentation

receiver/dockerstatsreceiver/logs_receiver.go

jamesmoessis · 2024-12-10T06:26:38Z

receiver/dockerstatsreceiver/logs_receiver.go

+		// event stream can be interrupted by async errors (connection or other).
+		// client caller must retry to restart processing. retry with backoff here
+		// except for context cancellation.
+		eventChan, errChan := d.client.Events(ctx, events.ListOptions{


What is the minimum Docker API version supported for these operations? If it is a higher version than otherwise documented in this receiver, it should be noted in the docs

i'm not sure about the minimum for events, but 1.25 for sure supports events:

https://docs.docker.com/reference/api/engine/version/v1.25/#tag/System/operation/SystemVersion

as for versions before that, i feel like we should keep 1.25 as the minimum for both receivers because it's already kind of outdated.

jamesmoessis · 2024-12-10T06:28:38Z

receiver/dockerstatsreceiver/logs_receiver.go

+}
+
+func (r *logsReceiver) Shutdown(_ context.Context) error {
+	if r.cancel != nil {


Consider a sync.WaitGroup to ensure the goroutine terminates synchronously with Shutdown()

spiffyy99 · 2024-12-18T17:48:10Z

updated integration tests and addressed all comments. might help to have a second reviewer as well, @ChrsMark could you take a look?

jamesmoessis

Nicely done! Thankyou for your contribution

jamesmoessis · 2024-12-19T04:09:54Z

@spiffyy99 - also feel free to add yourself as a codeowner for this component, if you wish. Looking for someone to review logging contributions going forward

spiffyy99 · 2024-12-19T04:45:23Z

@spiffyy99 - also feel free to add yourself as a codeowner for this component, if you wish. Looking for someone to review logging contributions going forward

Sure, i can do that.

github-actions bot added the receiver/dockerstats label Nov 8, 2024

github-actions bot requested a review from jamesmoessis November 8, 2024 22:45

spiffyy99 force-pushed the add-docker-stats-log-receiver branch 7 times, most recently from 8e96a6f to 735799c Compare November 14, 2024 04:57

spiffyy99 marked this pull request as ready for review November 14, 2024 04:57

spiffyy99 requested a review from a team as a code owner November 14, 2024 04:57

spiffyy99 requested a review from ChrsMark November 14, 2024 04:57

github-actions bot assigned jpkrohling Nov 14, 2024

spiffyy99 force-pushed the add-docker-stats-log-receiver branch 5 times, most recently from 4ec4ccf to 91c22b8 Compare November 14, 2024 12:40

github-actions bot added the Stale label Dec 8, 2024

jamesmoessis reviewed Dec 9, 2024

View reviewed changes

github-actions bot removed the Stale label Dec 10, 2024

jamesmoessis reviewed Dec 10, 2024

View reviewed changes

spiffyy99 force-pushed the add-docker-stats-log-receiver branch from 91c22b8 to 4308728 Compare December 13, 2024 20:44

spiffyy99 added 2 commits December 18, 2024 12:47

[dockerstatsreceiver] add new log receiver to ingest docker events

08d26b7

PR comments

1e9d043

spiffyy99 force-pushed the add-docker-stats-log-receiver branch from 4308728 to e2b2b87 Compare December 18, 2024 17:47

spiffyy99 force-pushed the add-docker-stats-log-receiver branch from e2b2b87 to d2661f6 Compare December 18, 2024 18:13

add integration tests

660b0b5

jamesmoessis approved these changes Dec 19, 2024

View reviewed changes

spiffyy99 force-pushed the add-docker-stats-log-receiver branch 3 times, most recently from f87f49e to 51e2a41 Compare December 19, 2024 17:36

add to codeowners

da811e1

spiffyy99 force-pushed the add-docker-stats-log-receiver branch from 51e2a41 to da811e1 Compare December 19, 2024 19:53

add print statement

076ef3e

spiffyy99 force-pushed the add-docker-stats-log-receiver branch from 7e07e33 to 076ef3e Compare December 19, 2024 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dockerstatsreceiver] add new log receiver to ingest docker events #36284

[dockerstatsreceiver] add new log receiver to ingest docker events #36284

spiffyy99 commented Nov 8, 2024 •

edited

Loading

spiffyy99 commented Nov 14, 2024 •

edited

Loading

spiffyy99 commented Nov 23, 2024

github-actions bot commented Dec 8, 2024

jamesmoessis commented Dec 9, 2024

jamesmoessis Dec 9, 2024

jamesmoessis Dec 9, 2024

spiffyy99 Dec 18, 2024

jamesmoessis left a comment

jamesmoessis Dec 10, 2024

spiffyy99 Dec 18, 2024

jamesmoessis Dec 10, 2024

spiffyy99 Dec 13, 2024

jamesmoessis Dec 10, 2024

spiffyy99 Dec 18, 2024

spiffyy99 commented Dec 18, 2024 •

edited

Loading

jamesmoessis left a comment

jamesmoessis commented Dec 19, 2024

spiffyy99 commented Dec 19, 2024


		- `endpoint` (default = `unix:///var/run/docker.sock`): Address to reach the desired Docker daemon.
		- `excluded_images` (no default, all running containers monitored): A list of strings,

[dockerstatsreceiver] add new log receiver to ingest docker events #36284

Are you sure you want to change the base?

[dockerstatsreceiver] add new log receiver to ingest docker events #36284

Conversation

spiffyy99 commented Nov 8, 2024 • edited Loading

Description

Link to tracking issue

Testing

Documentation

spiffyy99 commented Nov 14, 2024 • edited Loading

spiffyy99 commented Nov 23, 2024

github-actions bot commented Dec 8, 2024

jamesmoessis commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmoessis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spiffyy99 commented Dec 18, 2024 • edited Loading

jamesmoessis left a comment

Choose a reason for hiding this comment

jamesmoessis commented Dec 19, 2024

spiffyy99 commented Dec 19, 2024

spiffyy99 commented Nov 8, 2024 •

edited

Loading

spiffyy99 commented Nov 14, 2024 •

edited

Loading

spiffyy99 commented Dec 18, 2024 •

edited

Loading