chore: use testcontainers-go instead of containertest #16264

mdelapenya · 2022-11-11T12:27:22Z

Description:

This PR is replacing the internal package for managing containers lifecycle in tests with testcontainers-go. Following the same procedure as in #16209, where we updated the Redis receiver tests, we are updating:

zookeeper receiver
memcached receiver
docker stats receiver
aerospike receiver
kafka metrics receiver
docker observer extension

Link to tracking Issue: #9939

Testing:

I ran each module tests with:

cd $the_module
go test -timeout 600s -v -tags integration

Documentation: No docs were updated

Follow-ups
After merging this PR, I think the containertest package can be removed/deprecated.

mdelapenya · 2022-11-11T12:28:51Z

receiver/redisreceiver/go.mod

@@ -67,7 +67,5 @@ require (

 replace github.com/open-telemetry/opentelemetry-collector-contrib/internal/common => ../../internal/common

-replace github.com/open-telemetry/opentelemetry-collector-contrib/internal/containertest => ../../internal/containertest


I forgot to remove this line in #16209

mdelapenya · 2022-11-11T14:48:13Z

It seems the same error than yesterday happened at the windows pipeline. Is there anything I can do to troubleshoot it?

djaglowski · 2022-11-11T15:56:02Z

It seems the same error than yesterday happened at the windows pipeline. Is there anything I can do to troubleshoot it?

I opened #16267 to track the issue. I don't recall seeing this one in particular before, but we do deal with flaky tests much more frequently than we would like.

Ideally we would resolve every issue but these are often difficult to reproduce/debug. We try to at least document any failures in a corresponding issue and note reoccurrences as well. If nothing else, this provides more information about what may be happening. If a failure occurs frequently we will disable the test until it is resolved. We could certainly use more help with these flaky tests. Any help or ideas for managing are welcome.

djaglowski

LGTM. Thanks again @mdelapenya.

mdelapenya · 2022-11-11T16:30:30Z

The error seems a real one in dockerstatsreceiver.TestRemovedContainerRemovesRecordsIntegration.

Currently looking at it 👀

mdelapenya · 2022-11-11T16:47:37Z

@djaglowski I did my best trying to understand the test suite 🤞 Result is f01b5bd

I noticed the test failed because the assertions were expecting an isolated Docker execution, and it could be the case that other containers are running.

I'd recommend checking the current assertions regarding Docker events/stats, as they could be influenced by the existence of, i.e., the Ryuk container or other pending-to-be-pruned containers. Please take a look at f01b5bd#diff-ca81407f807d8cfe041b4a8ad51b991f51e0fcc20bfd8ff6bad32b9b9ac9e0ddR224 and f01b5bd#diff-ca81407f807d8cfe041b4a8ad51b991f51e0fcc20bfd8ff6bad32b9b9ac9e0ddR247, where I'm checking the initial count of running containers, and expecting just one more in the assertion.

Same issue in f01b5bd#diff-ca81407f807d8cfe041b4a8ad51b991f51e0fcc20bfd8ff6bad32b9b9ac9e0ddR255, where I created a stub Container from the container ID.

Hopefully I guessed correctly 😅

antonblock

Aerospike changes look good and pass integration tests

mdelapenya · 2022-11-11T18:51:47Z

Mongodb receiver tests failing:

--- FAIL: TestMongodbIntegration (0.00s)
    --- FAIL: TestMongodbIntegration/Running_mongodb_4.0 (16.87s)
    --- PASS: TestMongodbIntegration/Running_mongodb_4.2 (79.81s)
    --- PASS: TestMongodbIntegration/Running_mongodb_4.4_as_LPU (81.02s)
    --- PASS: TestMongodbIntegration/Running_mongodb_5.0 (76.70s)
FAIL
coverage: 73.3% of statements
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/mongodbreceiver	156.561s
?   	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/mongodbreceiver/internal/metadata	[no test files]
FAIL
make[1]: *** [../../Makefile.Common:68: do-integration-tests-with-cover] Error 1
make: *** [Makefile:188: receiver/mongodbreceiver] Error 2

We could probably be facing "integrated tests", or tests that could eventually depend on each other's state. Will take a look at it once I have time.

The container requests where using same fixed, exposed port, instead of using the random one. In this commit we are calculating the exposed port in the getContainer method, returning the endpoint for the container in order to be used by each test

mdelapenya · 2022-11-13T09:35:18Z

I noticed the mongoDB test examples were using a fixed port to be exposed, and that port was the same for mongo 4.0 and 4.2.

I updated the function to return the mongo container to retrieve both the container and its endpoint, calculating the dynamic, random port internally.

Hope this fixes the build 🤞

mdelapenya · 2022-11-13T10:39:48Z

The build failed because of the fpm install: https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/3454696043/jobs/5766407878

Run gem install --no-document fpm -v 1.11.0
  gem install --no-document fpm -v 1.11.0
  shell: /usr/bin/bash -e {0}
  env:
    TEST_RESULTS: testbed/tests/results/junit/results.xml
    SEGMENT_DOWNLOAD_TIMEOUT_MINS: 5
ERROR:  While executing gem ... (Gem::RemoteFetcher::FetchError)
    too many connection resets (https://rubygems.org/quick/Marshal.[4](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/3454696043/jobs/5766407878#step:4:4).[8](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/3454696043/jobs/5766407878#step:4:9)/fpm-1.11.0.gemspec.rz)
Error: Process completed with exit code 1.

Probably a retry on the step could help here: https://github.com/orgs/community/discussions/27121#discussioncomment-3254700

djaglowski · 2022-11-14T16:20:36Z

@mdelapenya, really appreciate your efforts on this.

Given that the mongo changes go beyond what is necessary for the others, would you be willing to open a separate PR for that component?

mdelapenya · 2022-11-15T06:40:25Z

Given that the mongo changes go beyond what is necessary for the others, would you be willing to open a separate PR for that component?

Sure, I can revert the commit for MongoDB and send it in a separate PR. I'm worried if, without the commit, the PR does not get merged because the mongo tests could eventually fail for the inconsistency of the used ports. If you agree with that, then't it's fine for me too :)

This reverts commit ffe8ca5.

@jamesmoessis

* main: [chore] dependabot updates Tue Nov 15 00:17:56 UTC 2022 (open-telemetry#16300) [receiver/rabbitmq] Fix flaky integration test (open-telemetry#16240) [Docker Stats Receiver] Add @jamesmoessis as code owner (open-telemetry#16253) [receiver/mongodbatlas] Alerts poll mode check hostname and port (open-telemetry#16286) [chore] update jaeger dep (open-telemetry#16287) [pkg/stanza] Fix issue where specifying a non-existent storage extension caused panic during shutdown. (open-telemetry#16213) [pkg/ottl] Rename ottllogs to ottllog (open-telemetry#16242) Register featuregate, otherwise does not work (open-telemetry#16269) [receiver/elasticsearch]: add fielddata memory size metrics on index level (open-telemetry#14922) [chore] remove what looks to be debugging code from instanaexporter (open-telemetry#16258)

…#16264) * chore: use testcontainers-go in zookeper receiver * chore: use testcontainers-go in memcached receiver * chore: use testcontainers-go in docker stats receiver * chore: use testcontainers-go in aerospike receiver * chore: use testcontainers-go in kafka metrics receiver * chore: use testcontainers-go in docker observer extension

mdelapenya added 7 commits November 11, 2022 13:23

chore: use testcontainers-go in zookeper receiver

a99efc7

fix: remove forgotten dependency in redis receiver

65120c6

chore: use testcontainers-go in memcached receiver

2597ff0

chore: use testcontainers-go in docker stats receiver

2435230

chore: use testcontainers-go in aerospike receiver

6e11db4

chore: use testcontainers-go in kafka metrics receiver

50f9b2b

chore: use testcontainers-go in docker observer extension

b2b98a3

mdelapenya requested a review from a team November 11, 2022 12:27

mdelapenya requested review from djaglowski and dmitryax as code owners November 11, 2022 12:27

github-actions bot assigned mx-psi Nov 11, 2022

github-actions bot added extension/observer receiver/aerospike receiver/dockerstats receiver/kafkametrics receiver/memcached receiver/redis Redis related issues receiver/zookeeper labels Nov 11, 2022

github-actions bot requested review from antonblock, pmcollins and rmfitzpatrick November 11, 2022 12:27

mdelapenya commented Nov 11, 2022

View reviewed changes

djaglowski added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 11, 2022

djaglowski approved these changes Nov 11, 2022

View reviewed changes

fix: consider the current Docker state in tests

f01b5bd

github-actions bot requested a review from djaglowski November 11, 2022 16:44

antonblock approved these changes Nov 11, 2022

View reviewed changes

fix: decouple mongoDB integration tests

ffe8ca5

The container requests where using same fixed, exposed port, instead of using the random one. In this commit we are calculating the exposed port in the getContainer method, returning the endpoint for the container in order to be used by each test

github-actions bot added the receiver/mongodb label Nov 13, 2022

github-actions bot requested review from antonblock and schmikei November 13, 2022 09:32

mdelapenya added 2 commits November 15, 2022 07:40

Revert "fix: decouple mongoDB integration tests"

f8b99ad

This reverts commit ffe8ca5.

github-actions bot requested a review from jamesmoessis November 15, 2022 06:46

mdelapenya mentioned this pull request Nov 15, 2022

fix: decouple mongoDB integration tests #16306

Merged

djaglowski approved these changes Nov 15, 2022

View reviewed changes

djaglowski merged commit ce37a30 into open-telemetry:main Nov 15, 2022

This was referenced Nov 15, 2022

Clarify usage between testcontainer and containertest #9939

Closed

[internal/containertest] Remove module #16315

Closed

mdelapenya deleted the receivers-testcontainers-go branch November 15, 2022 16:23

mdelapenya mentioned this pull request Nov 15, 2022

chore: retire containertest #16316

Merged

plantfansam mentioned this pull request Jul 21, 2023

disregard #24427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use testcontainers-go instead of containertest #16264

chore: use testcontainers-go instead of containertest #16264

mdelapenya commented Nov 11, 2022 •

edited

Loading

mdelapenya Nov 11, 2022

mdelapenya commented Nov 11, 2022

djaglowski commented Nov 11, 2022

djaglowski left a comment

mdelapenya commented Nov 11, 2022

mdelapenya commented Nov 11, 2022 •

edited

Loading

antonblock left a comment

mdelapenya commented Nov 11, 2022

mdelapenya commented Nov 13, 2022

mdelapenya commented Nov 13, 2022

djaglowski commented Nov 14, 2022 •

edited

Loading

mdelapenya commented Nov 15, 2022

		@@ -67,7 +67,5 @@ require (

		replace github.com/open-telemetry/opentelemetry-collector-contrib/internal/common => ../../internal/common

		replace github.com/open-telemetry/opentelemetry-collector-contrib/internal/containertest => ../../internal/containertest

chore: use testcontainers-go instead of containertest #16264

chore: use testcontainers-go instead of containertest #16264

Conversation

mdelapenya commented Nov 11, 2022 • edited Loading

mdelapenya Nov 11, 2022

Choose a reason for hiding this comment

mdelapenya commented Nov 11, 2022

djaglowski commented Nov 11, 2022

djaglowski left a comment

Choose a reason for hiding this comment

mdelapenya commented Nov 11, 2022

mdelapenya commented Nov 11, 2022 • edited Loading

antonblock left a comment

Choose a reason for hiding this comment

mdelapenya commented Nov 11, 2022

mdelapenya commented Nov 13, 2022

mdelapenya commented Nov 13, 2022

djaglowski commented Nov 14, 2022 • edited Loading

mdelapenya commented Nov 15, 2022

mdelapenya commented Nov 11, 2022 •

edited

Loading

mdelapenya commented Nov 11, 2022 •

edited

Loading

djaglowski commented Nov 14, 2022 •

edited

Loading