[chore] move dockerstats receiver's objects to package internal/docker/receiver

[aboguszewski-sumo]

Description: Functions and structs that could be reused have been moved to a separate package in order to be able to deprecate this receiver.
For reasons and arguments to do so, please see #32023 (comment)
Does it need a changelog entry? IMO not, it's not a change in the collector's config/api.
cc @andrzej-stencel

Link to tracking Issue: #31597

Testing: old tests, moved

Documentation: old documentation, moved or regenerated

[aboguszewski-sumo]

bump @andrzej-stencel

[aboguszewski-sumo]

I tried to investigate the failing checks, same thing happens here: https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/9086234384/job/24971438739?pr=33059

[MovieStoreGuy]

The scope of changes is far too high to be done within one PR.

I would suggest split the PR into at least two:

• Updated internal package with shared functionality
• Removing duplicated functionality

[aboguszewski-sumo]

While I partially agree, I'd like to make it clear whether this is the way it should be done, as purely transform dockerstats receiver into docker receiver is already taking a lot of time and there are some more changes I'd like to add.
WDYT @andrzej-stencel?

[andrzej-stencel]

I'm not sure if I understand correctly what @MovieStoreGuy is proposing. Is it to first duplicate the code from Docker Stats receiver into a common package (that would not be used anywhere?) and then separately remove the duplicated code from Docker Stats receiver, starting to use the common package?

If my understanding is correct, I'm not sure if this would make review easier for me. But if that would make reviewing easier for @MovieStoreGuy then it's probably the way to go.

[aboguszewski-sumo]

pinging @MovieStoreGuy, could you clarify here?

[jamesmoessis]

Looks OK, it definitely is a big change and is breaking so it's not super ideal, though I'm happy to move it along in the interest of getting things done

I also don't think you can be marking things like this as [chore] since it has actual code changes and even user facing changes to the output of the metrics.

[jamesmoessis]

This is technically a breaking change, and should be at least mentioned in the changelog.

[jamesmoessis]

Unsure how I feel about making all of these types and methods public. Is there an alternative to this?

For example, I can create the OTLP exporter with fairly minimal exposure to the public API.

t, err := otlphttpexporter.NewFactory().CreateTracesExporter(
		ctx,
		exporter.CreateSettings{
			ID:                component.NewID(component.MustNewType("mock_app_otlp_exporter")),
			TelemetrySettings: componenttest.NewNopTelemetrySettings(),
			BuildInfo:         component.NewDefaultBuildInfo(),
		},
		&otlphttpexporter.Config{
			ClientConfig: clientCfg,
			Encoding:     otlphttpexporter.EncodingProto,
		},
	)

Is there a way we could arrange this in a more effective way?

[aboguszewski-sumo]

I got an impression that this was necessary, but maybe this autoresolved at some point. Nevertheless, the PR is probably going to be split into smaller ones, so I will keep in mind your suggestion.

[MovieStoreGuy]

While I partially agree, I'd like to make it clear whether this is the way it should be done, as purely transform dockerstats receiver into docker receiver is already taking a lot of time and there are some more changes I'd like to add. WDYT @andrzej-stencel?

The process is slow because the amount of change is significantly high. If this was broken up in smaller PRs and incrementally moving packages and functionality then I could get behind it but as it stands this is ~11,000 additions.
It is too easy for something to go amiss or overlooked with this.

Is it to first duplicate the code from Docker Stats receiver into a common package (that would not be used anywhere?) and then separately remove the duplicated code from Docker Stats receiver, starting to use the common package?

It needs to be smaller and not a big bang, I don't mind how it is done but there needs to be more of a plan and reassurance that what is currently given.

My suggestion was to move the required code into a shared package so we can at least reduce the change set and effectively review what is happening, then once that is merged the new package can be adopt and old code and functionality can be removed.

In short, make the PRs smaller and easier to review and I'll be happy to review and approve as appropriate.

[aboguszewski-sumo]

There aren't many ways to break it into smaller chunks I can think of. One main problem is that there are many functions that use code generated by mdatagen. So, the only reasonable solution I have thought of is the following:

• edit: Zeroth PR (optional, imo can be merged with first one): prepare empty docker/receiver package
• First PR: move only internal/metadata package
• Second PR: move config
• Third PR: move metrics collecting logic
• Fourth PR: cleanup

Where maybe (second and third) or (third and fourth) can be merged into one.
cc @andrzej-stencel

[jamesmoessis]

@aboguszewski-sumo I have a question about the overall approach taken with the new docker events/logs gathering.

Why is the focus on moving docker stats receiver before creating the actual events receiver? I would've thought you create the dockerreceiver or dockereventsreceiver and then optionally move the metrics to it later. In my mind the question remains whether the metrics + events/logs need to be in the same component anyway, might it make sense to keep them separate? You might have more success more quickly if you weren't trying to make big changes to existing used components/code.

[aboguszewski-sumo]

@jamesmoessis This was something agreed quite early on: #31597 (comment)
While from time perspective I agree that your suggestion sounds better, initially this seemed like a better idea and I believe that @andrzej-stencel raised this on a SIG meeting.

In my mind the question remains whether the metrics + events/logs need to be in the same component anyway, might it make sense to keep them separate?

I believe this was asked on SIG meeting on March 6th, again Andrzej might elaborate.

From my perspective, it could be a pain to try to merge the receivers later on, as they could already have some differences in code structure. And again, we would be copying a lot of code, so there would be the same problem as in this PR. I already have a PoC of the events receiver so it is only a matter of adapting it to the final receiver that will be used.

[jamesmoessis]

Fair enough, I am just exploring possibilities for making it easier for you to get the new features you want faster.

[aboguszewski-sumo]

I am closing this PR as it's going to be superseded by other ones.