opentelemetry-collector-contrib
opentelemetry-collector-contrib copied to clipboard
open-telemetry
[chore] move dockerstats receiver's objects to package internal/docker/receiver
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 https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32023#discussion_r1547671474 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
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
Sorry for the autoping for everyone: I pushed this change to check if this will fix the failing CI (it's basically the result of running make gotidy)
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?
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.
While I partially agree, I'd like to make it clear whether this is the way it should be done, as purely transform
dockerstatsreceiver intodockerreceiver 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.
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/receiverpackage - First PR: move only
internal/metadatapackage - 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
@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.
@jamesmoessis This was something agreed quite early on: https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/31597#issuecomment-1981480643 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.
Fair enough, I am just exploring possibilities for making it easier for you to get the new features you want faster.
I am closing this PR as it's going to be superseded by other ones.