[dockerstatsreceiver] add new log receiver to ingest docker events #36284
Conversation
spiffyy99
force-pushed
the
add-docker-stats-log-receiver
branch
7 times, most recently
from
8e96a6f to
735799c
Compare
|
Ready for review for the most part, just need to update integration tests. |
spiffyy99
force-pushed
the
add-docker-stats-log-receiver
branch
5 times, most recently
from
4ec4ccf to
91c22b8
Compare
|
@jamesmoessis could you take a look at this? |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@spiffyy99 apologies for the delay, I will review this tomorrow. |
| @@ -35,9 +36,22 @@ func createDefaultConfig() component.Config { | |||
| Timeout: scs.Timeout, | |||
| }, | |||
| MetricsBuilderConfig: metadata.DefaultMetricsBuilderConfig(), | |||
| MinDockerRetryWait: time.Second, | |||
| MaxDockerRetryWait: 10 * time.Second, | |||
| Logs: EventsConfig{}, | |||
There was a problem hiding this comment.
Will this collect events automatically? I think it should be off-by-default as to not affect existing deployments with the receiver running
There was a problem hiding this comment.
Or will the log not be active unless they put it into a log pipeline... Good to know either way
There was a problem hiding this comment.
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.
|
|
||
| - `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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah what can be done in this case is creating a different receiver entirely. i'll mention this case in the documentation
| // 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{ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| func (r *logsReceiver) Shutdown(_ context.Context) error { | ||
| if r.cancel != nil { |
There was a problem hiding this comment.
Consider a sync.WaitGroup to ensure the goroutine terminates synchronously with Shutdown()
spiffyy99
force-pushed
the
add-docker-stats-log-receiver
branch
from
91c22b8 to
4308728
Compare
spiffyy99
force-pushed
the
add-docker-stats-log-receiver
branch
from
4308728 to
e2b2b87
Compare
|
updated integration tests and addressed all comments. might help to have a second reviewer as well, @ChrsMark could you take a look? |
spiffyy99
force-pushed
the
add-docker-stats-log-receiver
branch
from
e2b2b87 to
d2661f6
Compare
|
@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. |
spiffyy99
force-pushed
the
add-docker-stats-log-receiver
branch
from
d2661f6 to
d041096
Compare
Description
Adds a log receiver for
dockerstatsreceiverto ingest docker events. Adheres to the otel events standardsReceiver calls
client.Eventsto return channel and polls from channel. Retries connection to docker daemon upon error with exponential backoff by callingEventsmethod 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.