[receiver/dockerstatsreceiver] Initial PR to onboard dockerstats onto mdatagen #12322

jamesmoessis · 2022-07-12T07:13:55Z

First of all, apologies for the size of the PR. Thankfully, most of it is generated code/docs or mock/test data. Additionally, there is no change to the behaviour, since I haven't enabled the new implementation.

Description:
I wanted the initial PR to be backwards compatible with the existing implementation. So, I've re-implemented the scrape function as scrapeV2 (leaving the original) and testing them both alongside each other to ensure they produce the same metrics, and to give the reviewer confidence that feature parity is maintained. The bulk of the new implementation is all in receiver_v2.go.

I've defined the metrics to be recorded in metadata.yaml. With this, the new implementation (which isn't enabled yet) you can turn off and on each metric via the config, and all metrics have generated docs in documentation.md.

I've also fixed two bugs:

When collecting data from multiple containers, it seems that the resource metrics aren't appended but overridden, so the last container to be collected is the only one that you see metrics for. I made a minor change to fix this.
Specifying an HTTP endpoint (not unix socket) for the docker engine resulted in an incorrect dialer being used. It means it was ignoring the endpoint I specified and actually querying my local docker engine's unix socket. I had to make a minor change the to internal docker client to pass it an option, to make sure the correct dialer is being used.

Link to tracking Issue: #9794

Testing:
TestScrapes tests both implementations of scrape and ensures they emit the same metrics for the same datasource. I have mocked the docker engine using the HTTP test server.

Documentation:
mdatagen generates documentation in documentation.md.

Discussion points:
How should transition to using the new implementation by default? There are a few breaking changes that are needed to move the component forward here. With the two implementations, we could temporarily expose a config which allows users to stay on the "deprecated" implementation, giving them time to migrate, as we make the breaking changes to the new implementation of scrape only. Eventually, we delete the old implementation of scrape and a whole bunch of code with it. Would love thoughts and feedback here :)

…sted

…ce metrics

jamesmoessis · 2022-07-12T07:15:01Z

Hello @rmfitzpatrick @MovieStoreGuy can you please review :)

internal/docker/docker.go

receiver/dockerstatsreceiver/receiver.go

MovieStoreGuy

There is just a few things I have comments on atm, I will come back to review this in further detail.

receiver/dockerstatsreceiver/metadata.yaml

receiver/dockerstatsreceiver/receiver.go

mx-psi · 2022-07-15T08:42:46Z

@jamesmoessis Note that, per the ref doc

exclude directives only apply in the main module’s go.mod file and are ignored in other modules

So we need to add this:

on the main go.mod file
on the contrib releases manifest

jamesmoessis · 2022-07-18T02:11:09Z

Thanks @mx-psi for pointing that out. I have updated the main go.mod. I can also raise another PR in the releases repo.

Before I do that, though, I would like to ask your opinion on whether this should be a replace or an exclude? Both seem to work:

replace github.com/docker/go-connections v0.4.1-0.20210727194412-58542c764a11 => github.com/docker/go-connections v0.4.0

or

exclude github.com/docker/go-connections v0.4.1-0.20210727194412-58542c764a

I'm thinking it's possibly better as a replace now that I've properly read those ref docs you linked.

mx-psi · 2022-07-18T08:26:34Z

@jamesmoessis I am not sure how exclude interacts with pseudoversions, so maybe a replace is indeed better.

receiver/dockerstatsreceiver/documentation.md

MovieStoreGuy

I am happy with these changes, only nits that really shouldn't matter

MovieStoreGuy · 2022-07-20T03:01:50Z

internal/docker/docker.go

@@ -56,11 +56,13 @@ type Client struct {
 	logger               *zap.Logger
 }

-func NewDockerClient(config *Config, logger *zap.Logger) (*Client, error) {
+func NewDockerClient(config *Config, logger *zap.Logger, opts ...docker.Opt) (*Client, error) {


If we have reverted the go-connections version, does this still need to happen?
Or is this just a safe nice to have?

Yeah, it doesn't need to be there anymore, but imo it doesn't hurt to leave it in, and it could be nice to have for future purposes or similar issues. I don't feel strongly either way

receiver/dockerstatsreceiver/doc.go

receiver/dockerstatsreceiver/metadata.yaml

jamesmoessis · 2022-07-22T00:16:34Z

@rmfitzpatrick @mx-psi I believe I have addressed all comments, is there anything else I can do to move this PR forward?

mx-psi · 2022-07-22T07:43:22Z

Will wait for @rmfitzpatrick to give a final review

rmfitzpatrick

Thanks!

receiver/dockerstatsreceiver/receiver.go

mx-psi · 2022-07-25T11:25:14Z

@jamesmoessis Regarding

How should transition to using the new implementation by default?

I would suggest using a feature gate for this

jamesmoessis · 2022-07-25T23:27:36Z

@mx-psi thankyou for the suggestion, I will implement in my next PR!

…nnections` package The package `github.com/docker/go-connections` only exists in contrib's generated `go.mod` file in version `v0.5.0`, and does not exist in the replaced version `v0.4.1`. The distro also builds and runs fine without the replace directive. I believe the [reason](open-telemetry/opentelemetry-collector-contrib#12322 (comment)) for the replace directive to have been added is out of date. To be sure, I have run the repro scenario described [here](docker/go-connections#99 (comment)) and the behavior with go-connections v0.5.0 was correct - the Docker client tried to connect to the TCP port.

…nnections` package (#677) The package `github.com/docker/go-connections` only exists in contrib's generated `go.mod` file in version `v0.5.0`, and does not exist in the replaced version `v0.4.1`. The distro also builds and runs fine without the replace directive. I believe the [reason](open-telemetry/opentelemetry-collector-contrib#12322 (comment)) for the replace directive to have been added is out of date. To be sure, I have run the repro scenario described [here](docker/go-connections#99 (comment)) and the behavior with go-connections v0.5.0 was correct - the Docker client tried to connect to the TCP port. Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>

jamesmoessis added 20 commits June 27, 2022 12:45

initial setup for dockerstats mdatagen, with just memory metrics

2bfdcfc

add mdatagen metrics config option

dbea8b9

further updates to metadata.yaml

96a0518

initial alternate implementation of scrape()

86a2c6c

rearrange scraper so follows a stronger pattern and is more easily te…

ba3a74a

…sted

testing

b474d88

expected default metrics json

ddd4c43

add option to add more docker client opts

3ada5d6

work around nasty go-connections bug

e4406b4

remove unneeded

c9c00d6

create test for exist scraper while mocking docker engine

c1cbbc2

partway through onboarding more metrics to mdatagen

191b48b

add all remaining metrics to metadata.yaml and generate

f81c212

more detailed memory metrics

b4a2538

record more detailed mem metrics

da4b203

Merge branch 'main' into mdatagen-dockerstatsreceiver

178b39e

more mock testdata for docker engine

3d7eb46

final touch ups of metadata.yaml

c646c0f

tests for two containers in one engine for both scrape impls

121fa2d

fix bug where container metrics were overridden but subsequent resour…

a800ec5

…ce metrics

jamesmoessis commented Jul 12, 2022

View reviewed changes

internal/docker/docker.go Show resolved Hide resolved

jamesmoessis commented Jul 12, 2022

View reviewed changes

receiver/dockerstatsreceiver/receiver.go Show resolved Hide resolved

jamesmoessis commented Jul 12, 2022

View reviewed changes

receiver/dockerstatsreceiver/receiver.go Outdated Show resolved Hide resolved

fix search and replace error

e0be23c

MovieStoreGuy reviewed Jul 12, 2022

View reviewed changes

jamesmoessis added 4 commits July 13, 2022 11:01

implement moviestoreguy suggestions log + remove copy

8ff4540

implement moviestoreguy suggestion; move new implementation to new file

1955866

add const for default resource len

a12f0e7

appease linter

8cfeb4f

jamesmoessis added 3 commits July 15, 2022 09:44

pin go-connections to 0.4.0 and remove dialer override in client

2709245

Merge branch 'main' into mdatagen-dockerstatsreceiver

c51d3be

go mod tidy

a8f1cb0

exclude go-connections v0.4.1 in main go.mod

d7f4afd

replace instead of exclude

3750d20

jamesmoessis mentioned this pull request Jul 19, 2022

Replace bad go-connections version open-telemetry/opentelemetry-collector-releases#165

Merged

mx-psi reviewed Jul 19, 2022

View reviewed changes

receiver/dockerstatsreceiver/documentation.md Show resolved Hide resolved

MovieStoreGuy approved these changes Jul 20, 2022

View reviewed changes

jamesmoessis added 4 commits July 21, 2022 11:20

give more detail in device major/minor number attrs

a4aac5b

rearrange doc.go as per review

c396e60

Merge branch 'main' into mdatagen-dockerstatsreceiver

fb33b3b

go generate

e6d74f2

mx-psi requested a review from rmfitzpatrick July 22, 2022 07:43

rmfitzpatrick approved these changes Jul 22, 2022

View reviewed changes

mx-psi approved these changes Jul 25, 2022

View reviewed changes

mx-psi reviewed Jul 25, 2022

View reviewed changes

receiver/dockerstatsreceiver/receiver.go Show resolved Hide resolved

mx-psi merged commit a088337 into open-telemetry:main Jul 25, 2022

jamesmoessis deleted the mdatagen-dockerstatsreceiver branch July 25, 2022 23:27

jamesmoessis mentioned this pull request Jul 27, 2022

[receiver/dockerstats] Featuregate for new mdatagen implementation + semantic convention alignment #12743

Merged

andrzej-stencel mentioned this pull request Sep 25, 2024

[chore] refactor(contrib): remove unused replace directive for go-connections package open-telemetry/opentelemetry-collector-releases#677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/dockerstatsreceiver] Initial PR to onboard dockerstats onto mdatagen #12322

[receiver/dockerstatsreceiver] Initial PR to onboard dockerstats onto mdatagen #12322

jamesmoessis commented Jul 12, 2022 •

edited

Loading

jamesmoessis commented Jul 12, 2022

MovieStoreGuy left a comment

mx-psi commented Jul 15, 2022

jamesmoessis commented Jul 18, 2022 •

edited

Loading

mx-psi commented Jul 18, 2022

MovieStoreGuy left a comment

MovieStoreGuy Jul 20, 2022

jamesmoessis Jul 21, 2022 •

edited

Loading

jamesmoessis commented Jul 22, 2022

mx-psi commented Jul 22, 2022

rmfitzpatrick left a comment

mx-psi commented Jul 25, 2022

jamesmoessis commented Jul 25, 2022

[receiver/dockerstatsreceiver] Initial PR to onboard dockerstats onto mdatagen #12322

[receiver/dockerstatsreceiver] Initial PR to onboard dockerstats onto mdatagen #12322

Conversation

jamesmoessis commented Jul 12, 2022 • edited Loading

jamesmoessis commented Jul 12, 2022

MovieStoreGuy left a comment

Choose a reason for hiding this comment

mx-psi commented Jul 15, 2022

jamesmoessis commented Jul 18, 2022 • edited Loading

mx-psi commented Jul 18, 2022

MovieStoreGuy left a comment

Choose a reason for hiding this comment

MovieStoreGuy Jul 20, 2022

Choose a reason for hiding this comment

jamesmoessis Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

jamesmoessis commented Jul 22, 2022

mx-psi commented Jul 22, 2022

rmfitzpatrick left a comment

Choose a reason for hiding this comment

mx-psi commented Jul 25, 2022

jamesmoessis commented Jul 25, 2022

jamesmoessis commented Jul 12, 2022 •

edited

Loading

jamesmoessis commented Jul 18, 2022 •

edited

Loading

jamesmoessis Jul 21, 2022 •

edited

Loading