test: Enable race detector #15932

ericywl · 2025-02-28T03:19:06Z

Motivation/summary

Enable race detector for tests, continuation of #13827.

How to test these changes

Run unit test, system test and CI.

mergify · 2025-02-28T03:19:41Z

This pull request does not have a backport label. Could you fix it @ericywl? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-7.17 is the label to automatically backport to the 7.17 branch.
backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
backport-9./d is the label to automatically backport to the 9./d branch. /d is the digit.
backport-8.x is the label to automatically backport to the 8.x branch.
backport-active-all is the label that automatically backports to all active branches.
backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

carsonip

is the part that fixes flaky TestServerElasticsearchOutput related to the race detector? If no, I'd prefer it to be a separate PR to fix #13606 , as we might want to backport it to make our lives easier.

ericywl · 2025-02-28T15:04:10Z

is the part that fixes flaky TestServerElasticsearchOutput related to the race detector? If no, I'd prefer it to be a separate PR to fix #13606 , as we might want to backport it to make our lives easier.

Not exactly, I can extract that PR out.

simitt · 2025-03-03T07:31:01Z

internal/beater/server_test.go

+		"elasticsearch.events.count":  5,
+		"elasticsearch.events.queued": 5,
+		// Disable this flaky test to be fixed later
+		// "elasticsearch.bulk_requests.available": 9,


Can you please create a follow up task to fix this issue and link from here?
cc @raultorrecilla for scheduling work to look into why this is flaky.

@ericywl paste it here once created please!

Yup apparently its a long standing issue: #13606

@ericywl can you revert this change in this PR? The flaky test shouldn't block this PR. (Even if the bugfix isn't merged and it unfortunately fails, rerunning the CI should be good enough)

mergify · 2025-03-03T09:26:08Z

This pull request is now in conflicts. Could you fix it @ericywl? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b ci/test-race upstream/ci/test-race
git merge upstream/main
git push upstream ci/test-race

endorama · 2025-03-03T10:38:02Z

.github/workflows/ci.yml

@@ -55,16 +55,13 @@ jobs:
        with:
          go-version-file: go.mod
          cache: true
+      - run: go test -v -race ./...


Why are we removing CGO_ENABLED: "0" here?

Race detector unfortunately requires CGO to be enabled: https://go.dev/doc/articles/race_detector.

The race detector requires cgo to be enabled, and on non-Darwin systems requires an installed C compiler. The race detector supports linux/amd64, linux/ppc64le, linux/arm64, linux/s390x, freebsd/amd64, netbsd/amd64, darwin/amd64, darwin/arm64, and windows/amd64.

I think we explicitly set it to 0 to make sure we do not include any C based library. Has this change been discussed previously?

Hmmm, @kruskall I based this off your previous PR to enable the race detector. Not sure if that's something that has been discussed.

I confirmed with @kruskall that this is ok. systemtests run without CGO, so we are covered on that side.
I would add a comment to document this for future reference

kruskall · 2025-03-03T15:14:32Z

.github/workflows/ci.yml

@@ -55,16 +55,13 @@ jobs:
        with:
          go-version-file: go.mod
          cache: true
+      - run: go test -v -race ./...


suggestion: can you enable the race detector for the fips tests too ?

Sure, let me update it.

carsonip · 2025-03-03T17:19:24Z

docker-compose.yml

@@ -20,6 +20,7 @@ services:
      interval: 1s
    environment:
      - "ES_JAVA_OPTS=-Xms1g -Xmx1g"
+      - _JAVA_OPTIONS # Workaround on OSX for https://bugs.openjdk.org/browse/JDK-8345296


Q: Is the key _JAVA_OPTIONS without value good enough to workaround your issue? I don't see any mention of _JAVA_OPTIONS in the bug report.

It's mainly so that I can run the system tests using _JAVA_OPTIONS="-XX:UseSVE=0" make system-test without having to update the docker compose.

internal/beater/otlp/http.go

carsonip · 2025-03-03T17:21:28Z

internal/beater/otlp/http.go

@@ -57,8 +61,9 @@ func NewHTTPHandlers(logger *zap.Logger, processor modelpb.BatchProcessor, semap

 	// TODO we should add an otel counter metric directly in the
 	// apm-data consumer, then we could get rid of the callback.
+	metricRegistrationMu.Lock() // Temporary locking to fix data race issue


Also, is this a bugfix? If so, I prefer a separate PR, and if it is user-facing, it needs a changelog as well.

This has data race because the NewHTTPHandlers can be called by different goroutines concurrently in tests. I believe it's not user-facing since its only used internally in beater.

mergify · 2025-03-03T17:22:15Z

This pull request is now in conflicts. Could you fix it @ericywl? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b ci/test-race upstream/ci/test-race
git merge upstream/main
git push upstream ci/test-race

ericywl · 2025-03-04T04:11:03Z

System test failed previously due to Test_warmup but is suddenly passing again when I tried to look into it 🤡

EDIT: There seems to be another flaky system test, I will extract it into a new PR instead: #15992.

Enable race detector

a940e6e

ericywl mentioned this pull request Feb 28, 2025

test: enable race detector #13827

Closed

2 tasks

Add _JAVA_OPTIONS workaround and rename lock

9afeb77

ericywl force-pushed the ci/test-race branch 3 times, most recently from 40c948f to a6f9618 Compare February 28, 2025 07:00

ericywl marked this pull request as ready for review February 28, 2025 07:14

ericywl requested a review from a team as a code owner February 28, 2025 07:14

ericywl added the backport-skip Skip notification from the automated backport with mergify label Feb 28, 2025

carsonip reviewed Feb 28, 2025

View reviewed changes

Disable flaky elasticsearch output test temporarily

147ecd9

ericywl force-pushed the ci/test-race branch from b7461ac to 147ecd9 Compare February 28, 2025 15:08

1pkg previously approved these changes Mar 1, 2025

View reviewed changes

simitt reviewed Mar 3, 2025

View reviewed changes

Merge branch 'main' into ci/test-race

8828974

ericywl dismissed 1pkg’s stale review via 8828974 March 3, 2025 10:22

endorama reviewed Mar 3, 2025

View reviewed changes

kruskall reviewed Mar 3, 2025

View reviewed changes

carsonip reviewed Mar 3, 2025

View reviewed changes

Merge branch 'main' into ci/test-race

b728357

Add -race to fips test and use defer for unlock

9de5886

ericywl force-pushed the ci/test-race branch from 4a941fc to 9de5886 Compare March 4, 2025 06:50

ericywl and others added 3 commits March 5, 2025 10:49

Merge branch 'main' into ci/test-race

d12db58

Merge branch 'main' into ci/test-race

9b1c427

Add comment about CGO on system-test

a0e2175

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Enable race detector #15932

test: Enable race detector #15932

ericywl commented Feb 28, 2025

mergify bot commented Feb 28, 2025

carsonip left a comment

ericywl commented Feb 28, 2025

simitt Mar 3, 2025

raultorrecilla Mar 3, 2025

ericywl Mar 3, 2025

carsonip Mar 3, 2025

ericywl Mar 4, 2025

mergify bot commented Mar 3, 2025

endorama Mar 3, 2025

ericywl Mar 3, 2025

endorama Mar 3, 2025

ericywl Mar 4, 2025

endorama Mar 4, 2025

kruskall Mar 3, 2025

ericywl Mar 4, 2025

carsonip Mar 3, 2025

ericywl Mar 4, 2025

carsonip Mar 3, 2025

ericywl Mar 4, 2025 •

edited

Loading

mergify bot commented Mar 3, 2025

ericywl commented Mar 4, 2025 •

edited

Loading

test: Enable race detector #15932

Are you sure you want to change the base?

test: Enable race detector #15932

Conversation

ericywl commented Feb 28, 2025

Motivation/summary

How to test these changes

mergify bot commented Feb 28, 2025

carsonip left a comment

Choose a reason for hiding this comment

ericywl commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericywl Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

mergify bot commented Mar 3, 2025

ericywl commented Mar 4, 2025 • edited Loading

ericywl Mar 4, 2025 •

edited

Loading

ericywl commented Mar 4, 2025 •

edited

Loading