Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracer TestUserMonitoring: Wait for goroutines before test completes #2263

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

evanj
Copy link
Contributor

@evanj evanj commented Oct 11, 2023

What does this PR do?

This test starts a set of goroutines which do things to spans after they have finished. Wait for these goroutines to complete before ending the test function. This ensures these goroutines are not running when other tests run. This avoids these tests accidentally capturing the logs from these goroutines, and avoids the race detector triggering on things they are doing.

This fixes a problem when running tests with -shuffle=on, and also fixes a race detector warning:

==================
WARNING: DATA RACE
Write at 0x000103d80470 by goroutine 74703:
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.setTestTime()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/abandonedspans_test.go:28 +0xf8
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestReportAbandonedSpans.func2()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/abandonedspans_test.go:81 +0x44
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1648 +0x40

Previous read at 0x000103d80470 by goroutine 74356:
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*span).Finish()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/span.go:423 +0x3c
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestUserMonitoring.func5.2()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/tracer_test.go:2331 +0xc4

Goroutine 74703 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1648 +0x5d8
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestReportAbandonedSpans()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/abandonedspans_test.go:79 +0x204
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1648 +0x40

Goroutine 74356 (running) created at:
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestUserMonitoring.func5()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/tracer_test.go:2329 +0x154
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1648 +0x40
==================

Motivation

I'm trying to add a test and bug fix for a race detector bug, and I also found this race detector bug in the main branch tests.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

This test starts a set of goroutines which do things to spans after
they have finished. Wait for these goroutines to complete before
ending the test function. This ensures these goroutines are not
running when other tests run. This avoids these tests accidentally
capturing the logs from these goroutines, and avoids the race
detector triggering on things they are doing.

This fixes a problem when running tests with -shuffle=on, and also
fixes a race detector warning:

==================
WARNING: DATA RACE
Write at 0x000103d80470 by goroutine 74703:
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.setTestTime()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/abandonedspans_test.go:28 +0xf8
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestReportAbandonedSpans.func2()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/abandonedspans_test.go:81 +0x44
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1648 +0x40

Previous read at 0x000103d80470 by goroutine 74356:
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*span).Finish()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/span.go:423 +0x3c
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestUserMonitoring.func5.2()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/tracer_test.go:2331 +0xc4

Goroutine 74703 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1648 +0x5d8
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestReportAbandonedSpans()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/abandonedspans_test.go:79 +0x204
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1648 +0x40

Goroutine 74356 (running) created at:
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestUserMonitoring.func5()
      /Users/evan.jones/dd/dd-trace-go/ddtrace/tracer/tracer_test.go:2329 +0x154
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.2/libexec/src/testing/testing.go:1648 +0x40
==================
@evanj evanj requested a review from a team October 11, 2023 20:46
@pr-commenter
Copy link

pr-commenter bot commented Oct 11, 2023

Benchmarks

Benchmark execution time: 2023-10-12 14:08:12

Comparing candidate commit 606710f in PR branch evan.jones/usermonitoring-waitgroup with baseline commit f72e6dd in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics.

felixge added a commit that referenced this pull request Oct 12, 2023
@felixge felixge mentioned this pull request Oct 12, 2023
5 tasks
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks for reporting and fixing. One small change is needed, but otherwise this LGTM.

There might be a few follow-up tasks here:

  1. Set -shuffle=on in our CI: ci: run tests with -shuffle=on #2265
  2. Start adopting https://github.com/uber-go/goleak on the package and perhaps also individual test level.

I'll put these items on the agenda for our next dd-trace-go guild meeting.

ddtrace/tracer/tracer_test.go Outdated Show resolved Hide resolved
@felixge felixge enabled auto-merge (squash) October 12, 2023 13:57
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Enabling auto-merge. Still needs review from @DataDog/tracing-go team. Also re-running CI, seems like there was a flake.

@felixge felixge merged commit b0abc76 into main Oct 12, 2023
52 checks passed
@felixge felixge deleted the evan.jones/usermonitoring-waitgroup branch October 12, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants