opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard

New component: Docker receiver

Open aboguszewski-sumo opened this issue 1 year ago • 4 comments

Description: This PR adds a new Docker receiver that is going to replace Docker Stats receiver.

Full list of changes between Docker stats receiver and Docker receiver (as of now):

  • The name ;)
  • Config has undergone some changes: stats key was added for options that were related only to fetching stats.
  • I have added myself as a codeowner, alongside the codeowners for Docker stats receiver

I agree that this PR is quite big, but it's mostly generated or copied code.

The receiver is going to eventually support events and container logs, they will be added in separate pull requests.

I wasn't sure if I should deprecate dockerstatsreceiver in this PR or in a separate one - please let me know.

cc @astencel-sumo

Link to tracking Issue: Please see comments in #31597.

Testing: Tests were refactored to match the new config.

Documentation: Mostly copied from original receiver, some changes related to config occured.

aboguszewski-sumo avatar Mar 28 '24 14:03 aboguszewski-sumo

Also can the code owners of the original Docker Stats receiver please take a look :pray: @rmfitzpatrick @jamesmoessis

andrzej-stencel avatar Apr 02 '24 11:04 andrzej-stencel

If you think the name dockerstats is a problem then I would suggest looking at renaming it.

This is exactly what we want to do here. See https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/31597#issuecomment-1985378982:

Renaming a component in a graceful manner is a non-trivial endeavor. We did something similar with changing logging exporter to debug. How this would work in practice is perhaps:

  1. Create a new docker receiver that will share the metrics collection code with the existing docker_stats receiver. Deprecate the docker_stats receiver in favor of the docker receiver.
  2. Add the new code for logs collection (Docker events, container logs - possibly in separate PRs) in the new docker receiver, leaving the old docker_stats receiver's functionality unchanged.
  3. Remove the deprecated docker_stats receiver after a long time, perhaps 6 or 12 months.

We could indeed just rename the component and release a new version of the collector that does not have docker_stats receiver and instead has docker receiver. We are choosing a different path, that is harder for us - maintainers - but hopefully easier for users of the collector and also vendors of other distributions. We want to prevent unexpected breaking changes as much as possible, especially for popular components, even when the component is in "Alpha" stability.

By "we are choosing" I'm not sure whom I mean besides me :wink: So if there are other maintainers who think the simple rename procedure is fine, I'm open to changing it. I'll actually bring it to today's SIG meeting.

andrzej-stencel avatar Apr 10 '24 07:04 andrzej-stencel

By "we are choosing" I'm not sure whom I mean besides me 😉 So if there are other maintainers who think the simple rename procedure is fine, I'm open to changing it. I'll actually bring it to today's SIG meeting.

We discussed this in the SIG meeting on 2024-04-10 and the consensus was that the component should be renamed gradually.

andrzej-stencel avatar Apr 16 '24 07:04 andrzej-stencel

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

github-actions[bot] avatar May 01 '24 05:05 github-actions[bot]

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

github-actions[bot] avatar May 29 '24 05:05 github-actions[bot]

I'm closing this for now: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/33038 has higher priority and I will recreate/reopen this PR once it gets accepted.

aboguszewski-sumo avatar Jun 11 '24 10:06 aboguszewski-sumo