[receiver/riak] Riak Metric Receiver

[armstrmi]

Description: Adding New Component: Riak Metric Receiver,

Link to tracking Issue: New Component: Riak Metric Receiver

Testing: series of testing was added including

• config_test.go
• client_test.go
• scraper_test.go
• factory_test.go
  As well as sample configs for running local riak receiver and verify that metrics are being received

Documentation: README.md

[djaglowski]

Overall looks quite good. A few things need another look though.

We also need to add an integration test like this one.

[djaglowski]

Please add this module to the CODEOWNERS file with the two of us listed as owners.

[djaglowski]

Please add the component to the root go.mod (include a replace directive) and also cmd/configschema tests.

[djaglowski]

@armstrmi CI is still failing on this:

-package riakreceiver
+package riakreceiver // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/riakreceiver"
 
 import (
 	"context"
Porto links are out of date, please run "make goporto" and commit the changes in this PR.
Error: Process completed with exit code 1.

[djaglowski]

Generated code is out of date, please run "make generate" and commit the changes in this PR.
Error: Process completed with exit code 1.

[djaglowski]

👍 The integration test is running successfully:

make[2]: Entering directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/riakreceiver'
running go integration test ./... + coverage in /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/riakreceiver
go: downloading github.com/open-telemetry/opentelemetry-collector-contrib/internal/scrapertest v0.47.0
go: downloading go.opentelemetry.io/otel v1.5.0
go: downloading go.opentelemetry.io/otel/trace v1.5.0
go: downloading github.com/docker/docker v20.10.11+incompatible
go: downloading github.com/go-logr/logr v1.2.2
go: downloading google.golang.org/protobuf v1.27.1
=== RUN   TestRiakIntegration
2022/03/29 15:43:26 Starting container id: 7a[713](https://github.com/open-telemetry/opentelemetry-collector-contrib/runs/5741948997?check_suite_focus=true#step:7:713)83af5d7 image: testcontainers/ryuk:0.3.3
2022/03/29 15:43:26 Waiting for container id 7a71383af5d7 image: testcontainers/ryuk:0.3.3
2022/03/29 15:43:26 Container is ready id: 7a71383af5d7 image: testcontainers/ryuk:0.3.3
2022/03/29 15:44:10 Starting container id: 76ec36cf280c image: fd[730](https://github.com/open-telemetry/opentelemetry-collector-contrib/runs/5741948997?check_suite_focus=true#step:7:730)e39-81f1-49f9-b737-8ad62c6435ad:d6f37d0a-607f-42f0-a59d-f91cc9b4019e
2022/03/29 15:44:10 Waiting for container id 76ec36cf280c image: fd730e39-81f1-49f9-b737-8ad62c6435ad:d6f37d0a-607f-42f0-a59d-f91cc9b4019e
2022/03/29 15:44:14 Container is ready id: 76ec36cf280c image: fd730e39-81f1-49f9-b737-8ad62c6435ad:d6f37d0a-607f-42f0-a59d-f91cc9b4019e
--- PASS: TestRiakIntegration (56.54s)
PASS
coverage: 68.5% of statements
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/riakreceiver	56.604s	coverage: 68.5% of statements
?   	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/riakreceiver/internal/metadata	[no test files]
?   	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/riakreceiver/internal/mocks	[no test files]
?   	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/riakreceiver/internal/model	[no test files]
make[2]: Leaving directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/riakreceiver'

[djaglowski]

LGTM. Thanks @armstrmi!

[jpkrohling]

I'm waiting on the new release to be performed before merging this. It would probably cause this PR to get outdated (like, the changelog and go.mod/sum).

[anuraaga]

I appreciate the thumbs up from the contributors but I definitely wish that maintainers instead took the effort to update existing PRs when they make changes that break them, including releases.

[jpkrohling]

I agree with you, @anuraaga. I think most of us are trying that, but there are cases where it's not possible/feasible. In this case, the release hasn't been made and I was about to leave for the day, but wanted to make it clear (also to other maintainers) that this wouldn't get merged before the release.

[anuraaga]

Cool sounds like the sentiment is there.

[jpkrohling]

Forgot another case where we can't do it (which is the case with this PR as well): if contributors don't mark the PR as "maintainers can change it", we can't add commits or force push a change to the PR. The best we can then do is push the branch to our own forks, and offer it to the contributor.

[pmm-sumo]

As @djaglowski noticed, changelog needs to be cleaned up from the duplicate entries

[bogdandrutu]

@djaglowski @jpkrohling do we have an issue for this?

Update: Saw the issue in the description. don't know how you folks can carefully review 5500 lines in a reasonable amount of time :)) probably you spent lots of time...

[armstrmi]

@bogdandrutu yes we do, #8364

[djaglowski]

don't know how you folks can carefully review 5500 lines in a reasonable amount of time :)) probably you spent lots of time...

It can easily be done in an unreasonable amount of time :)

More seriously, my strategy has been to approach it as multiple review sessions. Start with obvious issues / gaps. Then focus on config and data model. Then factory and scraper. Double check tests and coverage. Finally checking all minor details.

Necessary changes to address feedback were simple and clear and have been completed.