chore: Update Testcontainers for .NET to version 2.3.0

[HofmeisterAn]

What does this PR do?

Updates the Testcontainers for .NET NuGet dependency to its latest release 2.3.0.

Why is it important?

I noticed you are using an outdated version. The old versions are not compatible with Docker Desktop anymore.

Related issues

• Relates to Use Testcontainers v2.2.0 in Elastic.Apm.Elasticsearch.Tests #1936

[cla-checker-service]

💚 CLA has been signed

[github-actions]

👋 @HofmeisterAn Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-12-23T21:21:07.581+0000

• Duration: 83 min 56 sec

Test stats 🧪

Test	Results
Failed	0
Passed	22269
Skipped	175
Total	22444

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark test.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[z1c0]

Hi @HofmeisterAn,

thanks for your contribution!
Could please you take a look at our CLA and sign it so we can proceed with this PR?

[HofmeisterAn]

run elasticsearch-ci/docs

[z1c0]

Hi @HofmeisterAn,

unfortunately, a couple of tests are not passing anymore with your changes applied.

Could you please have a look and let me know whether we should proceed with this PR?

Thanks
Wolfgang

[HofmeisterAn]

Those tests failed right? I can take a look at them.

[z1c0]

Those tests failed right? I can take a look at them.

Yes, thank you!
Ignore the first one (OpenTelemetryTests.MixApisTest2) though, this one appears to be just flaky.

[HofmeisterAn]

@z1c0 I've applied two fixes.

• Elastic.Clients.Elasticsearch.Tests: It does not make sense to validate against a hard coded value localhost. This value varies according to the Docker environment. The best practice is always to rely on TC Hostname property. TC takes care to resolve the right value accordingly to the Docker environment.
• Elastic.Apm.Profiler.Managed.Tests: The Oracle container just requires a password to start.

BTW, are there any best practices to configure Docker (using WSL2) in combination with the max_map_count property? First I ran again into testcontainers/testcontainers-dotnet#640 (comment).

[gregkalapos]

Hi @HofmeisterAn

thank you so much for opening this!

Just a side-note: In the related issue (#1936), I meant that in the Elastic.Apm.Elasticsearch.Tests project classes like ElasticsearchTestContainerConfiguration.cs and ElasticsearchTestContainer can be completely removed and we could use DotNet.Testcontainers.Containers.ElasticsearchTestcontainer now. Those tests predate DotNet.Testcontainers.Containers.ElasticsearchTestcontainer, that's why we have those classes, but now we could drop them and just rely on ElasticsearchTestcontainer from DotNet.Testcontainers.

This PR already provides value without that, so I think we'll be fine merging without that. Just wanted to let you know, in case you have bandwidth for it. :)

Thanks again!

[HofmeisterAn]

I meant that in the Elastic.Apm.Elasticsearch.Tests project classes like ElasticsearchTestContainerConfiguration.cs and ElasticsearchTestContainer can be completely removed and we could use DotNet.Testcontainers.Containers.ElasticsearchTestcontainer now.

@gregkalapos Yep, I can create a follow-up PR, but it looks like you are using a slightly different configuration:

https://github.com/elastic/apm-agent-dotnet/blob/61b13dfdf064f9a347d7e03773a0281e64133661/test/Elastic.Apm.Elasticsearch.Tests/ElasticsearchTestContainerConfiguration.cs#L41-L42C10

Does it make sense to add those changes to the upstream repository?

[gregkalapos]

I meant that in the Elastic.Apm.Elasticsearch.Tests project classes like ElasticsearchTestContainerConfiguration.cs and ElasticsearchTestContainer can be completely removed and we could use DotNet.Testcontainers.Containers.ElasticsearchTestcontainer now.

@gregkalapos Yep, I can create a follow-up PR, but it looks like you are using a slightly different configuration:

https://github.com/elastic/apm-agent-dotnet/blob/61b13dfdf064f9a347d7e03773a0281e64133661/test/Elastic.Apm.Elasticsearch.Tests/ElasticsearchTestContainerConfiguration.cs#L41-L42C10

Does it make sense to add those changes to the upstream repository?

None of that is critical. Environments.Add("discovery.type", "single-node"); just turns off discovery of other instances to form a multi nodes cluster. We do it because we only start 1 instance of elasticsearch which we test against. The default is multi-node, so multiple instances can form a cluster - if there is only 1 instance, it'll still work fine. So leaving it on default is fine.

Next line just makes sure elasticsearch is up and running before we run the test - that's already included in the upstream if I remember correctly.

Only thing we'll need is to make sure we can run elasticsearch version 7.x (latest is 8.x, here we ideally wanna test against 7.x).

[HofmeisterAn]

Next line just makes sure elasticsearch is up and running before we run the test - that's already included in the upstream if I remember correctly.

Upstream just checks the availability of the port.

Only thing we'll need is to make sure we can run elasticsearch version 7.x (latest is 8.x, here we ideally wanna test against 7.x).

Overriding the image version is not a problem.

[HofmeisterAn]

@z1c0 @gregkalapos regarding the latest build failures I need your help. What is the correct image for the Elastic.Clients.Elasticsearch.Tests tests? It looks like TC cannot find the image docker.elastic.co/elasticsearch/elasticsearch:8.5. I switched those two lines:

apm-agent-dotnet/test/Elastic.Clients.Elasticsearch.Tests/ElasticsearchTestFixture.cs

Lines 23 to 24 in 7bfcf46

	.WithDatabase(_configuration)
	.WithImage("docker.elastic.co/elasticsearch/elasticsearch:8.5")

Previous the build pulled elasticsearch:8.3.2 from Docker Hub (default TC module configuration).

---

For the Oracle tests I am not sure if the test assertions are right, this line fails:

apm-agent-dotnet/test/Elastic.Apm.Profiler.Managed.Tests/AdoNet/OracleManagedDataAccessCoreCommandTests.cs

Line 66 in 89cd545

apmServer.ReceivedData.Spans.Should().HaveCount(AdoNetTestData.DbRunnerExpectedTotalSpans + AdoNetTestData.OracleProviderExpectedSpans);

• apmServer.ReceivedData.Spans == 179
• AdoNetTestData.DbRunnerExpectedTotalSpans == 179
• AdoNetTestData.OracleProviderExpectedSpans == 63

OC, removing the addition of AdoNetTestData.OracleProviderExpectedSpans fix the test, but I am not sure if that is intended. Can someone help (besides that the test runs fine)?

[z1c0]

What is the correct image for the Elastic.Clients.Elasticsearch.Tests tests? It looks like TC cannot find the image docker.elastic.co/elasticsearch/elasticsearch:8.5. I switched those two lines:

docker.elastic.co/elasticsearch/elasticsearch:8.5.0 should work.

OC, removing the addition of AdoNetTestData.OracleProviderExpectedSpans fix the test, but I am not sure if that is intended. Can someone help (besides that the test runs fine)?

@gregkalapos do you have any experience with those Oracle tests? Are the span counts supposed to change? I would assume not.

[gregkalapos]

What is the correct image for the Elastic.Clients.Elasticsearch.Tests tests? It looks like TC cannot find the image docker.elastic.co/elasticsearch/elasticsearch:8.5. I switched those two lines:

docker.elastic.co/elasticsearch/elasticsearch:8.5.0 should work.

Yeah, any 8.x is fine.

OC, removing the addition of AdoNetTestData.OracleProviderExpectedSpans fix the test, but I am not sure if that is intended. Can someone help (besides that the test runs fine)?

@gregkalapos do you have any experience with those Oracle tests? Are the span counts supposed to change? I would assume not.

Me neither - those should not change. I'm looking into this.

[HofmeisterAn]

I assume docker.elastic.co is a private registry. TC does not find the credentials:

[testcontainers.org 00:00:01.29] Docker config "/var/lib/jenkins/workspace/net_apm-agent-dotnet-mbp_PR-1953/apm-agent-dotnet/.docker/config.json" not found
[testcontainers.org 00:00:01.29] Searching Docker registry credential in Auths
[testcontainers.org 00:00:01.29] Docker registry credential docker.elastic.co not found

[gregkalapos]

I assume docker.elastic.co is a private registry. TC does not find the credentials:

[testcontainers.org 00:00:01.29] Docker config "/var/lib/jenkins/workspace/net_apm-agent-dotnet-mbp_PR-1953/apm-agent-dotnet/.docker/config.json" not found
[testcontainers.org 00:00:01.29] Searching Docker registry credential in Auths
[testcontainers.org 00:00:01.29] Docker registry credential docker.elastic.co not found

Is that locally on your machine? We have it already in our code. I suspect it worked for me locally, because my local docker was logged in and we have the credentials in CI.

In any case, I don't think it's related to this PR. We can use any 8.x image. Maybe we could use this one.

So if it's only locally for you, I'd suggest ignore it for now and maybe file an issue (I agree it's not ideal), or switch to a public image.

[HofmeisterAn]

Is that locally on your machine? We have it already in our code. I suspect it worked for me locally, because my local docker was logged in and we have the credentials in CI.

So if it's only locally for you, I'd suggest ignore it for now and maybe file an issue (I agree it's not ideal), or switch to a public image.

For the local tests I switched to the public repository, which runs fine. But it fails on your Jenkins too. See the Docker config path (DOCKER_CONFIG):

Docker config "/var/lib/jenkins/workspace/net_apm-agent-dotnet-mbp_PR-1953/apm-agent-dotnet/.docker/config.json" not found

Not sure if PRs have access.

[gregkalapos]

Is that locally on your machine? We have it already in our code. I suspect it worked for me locally, because my local docker was logged in and we have the credentials in CI.

So if it's only locally for you, I'd suggest ignore it for now and maybe file an issue (I agree it's not ideal), or switch to a public image.

For the local tests I switched to the public repository, which runs fine. But it fails on your Jenkins too. See the Docker config path (DOCKER_CONFIG):

Docker config "/var/lib/jenkins/workspace/net_apm-agent-dotnet-mbp_PR-1953/apm-agent-dotnet/.docker/config.json" not found

Not sure if PRs have access.

I see. But I still don't understand why it doesn't work now and why it worked before. We use docker.elastic.co/elasticsearch/elasticsearch:8.5 on main and that works both in CI and locally for me.

Not sure if PRs have access.

PR branch should not make a difference.

[HofmeisterAn]

I see. But I still don't understand why it doesn't work now and why it worked before.

It worked before, because it NOT used the private Docker registry docker.elastic.co, it used Docker Hub instead. I changed the order because I thought you would like to test your own images. We can simply remove .WithImage("docker.elastic.co/elasticsearch/elasticsearch:8.5") if you are fine with the public version.

[gregkalapos]

I see. But I still don't understand why it doesn't work now and why it worked before.

It worked before, because it NOT used the private Docker registry docker.elastic.co, it used Docker Hub instead. I changed the order because I thought you would like to test your own images.

Really? Ok, I did not know that. So you say this line overwrites the line before with WithImage("docker.elastic.co/elasticsearch/elasticsearch:8.5") and pulls the default image?

We can simply remove .WithImage("docker.elastic.co/elasticsearch/elasticsearch:8.5") if you are fine with the public version.

Yes, we are fine with that, all I wanted to make sure in those tests is that we pull any 8.x version (and not 7.x), but now I learn that line did not do anything anyway :)

Yeah, so in general, we don't care about the exact version and where the image is coming from. All we want is any 8.x version and a reliable repo to pull from (Docker Hub is fine).

[HofmeisterAn]

Really? Ok, I did not know that. So you say this line overwrites the line before with WithImage("docker.elastic.co/elasticsearch/elasticsearch:8.5") and pulls the default image?

Yep, excatly 😀 The new module API will make it much more convenient in the future — looking forward to finish it next year. Then I will remove it and we pull the image from Docker Hub 👍.

[gregkalapos]

I looked into the failing oracle tests - I think our original intuition was wrong: the number of spans changes indeed and that seems to be ok to me.

So, how to proceed with that part?
@HofmeisterAn what you suggested is fine:

OC, removing the addition of AdoNetTestData.OracleProviderExpectedSpans fix the test, but I am not sure if that is intended.

The behavior in Oracle seems to change and we can adjust our asserts. Just remove AdoNetTestData.OracleProviderExpectedSpans.

Here are my findings:

I attached the test-case to a real APM setup. On the left side you see main, on the right side you see the result from this PR:

As you can see there are a whole bunch of spans called DECLARE on main (left side), which are not present when I run it from this PR (right side). The db statement for it is this:

DECLARE  err_code VARCHAR2(2000);  err_msg VARCHAR2(2000);  BEGIN  SELECT VALUE into :p_nls_comp from nls_session_parameters where PARAMETER='NLS_COMP'; SELECT VALUE into :p_nls_length_semantics from nls_session_parameters where PARAMETER='NLS_LENGTH_SEMANTICS'; SELECT VALUE into :p_nls_nchar_conv_excep from nls_session_parameters where PARAMETER='NLS_NCHAR_CONV_EXCP'; SELECT '0' into :p_err_code from dual;  SELECT '0' into :p_err_msg from dual;  END;

This does not seem to come from our test application - looks like some generated query and it has something to do with session_parameters.

This is the code (including the comment) which counts these additional spans:

// frequent commands are executed to retrieve session parameters. we should ignore these
public const int OracleProviderExpectedSpans = 63;

I suspect TC updated the oracle version in the new release (so it pulls another image with a newer oracle version) and this oracle version does not generate these db queries. The rest of the asserts and the generated spans all look good to me.

So let's just remove public const int OracleProviderExpectedSpans = 63; and we are good to go on this part.

[HofmeisterAn]

I suspect TC updated the oracle version in the new release (so it pulls another image with a newer oracle version)

Yes, TC for .NET updated Oracle from oracle-xe-11g-r2 to oracle-xe:21-slim.

So let's just remove public const int OracleProviderExpectedSpans = 63; and we are good to go on this part.

I removed it and pushed the changes.

[gregkalapos]

Thank you @HofmeisterAn!