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

Fix race when reporting internal cardinality metrics #257

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Conversation

vdarulis
Copy link
Collaborator

@vdarulis vdarulis commented Apr 12, 2024

Previous attempt to fix it was incomplete - added a test that reliably triggers this.
Note that there was no need to track interm counts atomically, the subscope iterator is executed serially.

==================
WARNING: DATA RACE
Write at 0x00c000194588 by goroutine 8:
  github.com/uber-go/tally/v4.(*scope).Gauge()
      /Users/vytenis/go/tally/scope.go:341 +0x3a4
  github.com/uber-go/tally/v4.TestCachedReporterInternalMetricsConcurrent.func2()
      /Users/vytenis/go/tally/scope_registry_test.go:293 +0x114

Previous read at 0x00c000194588 by goroutine 9:
  github.com/uber-go/tally/v4.(*scopeRegistry).reportInternalMetrics.func1()
      /Users/vytenis/go/tally/scope_registry.go:314 +0xe0
  github.com/uber-go/tally/v4.(*scopeRegistry).ForEachScope()
      /Users/vytenis/go/tally/scope_registry.go:159 +0x118
  github.com/uber-go/tally/v4.(*scopeRegistry).reportInternalMetrics()
      /Users/vytenis/go/tally/scope_registry.go:310 +0x1e4
  github.com/uber-go/tally/v4.(*scopeRegistry).CachedReport()
      /Users/vytenis/go/tally/scope_registry.go:137 +0x70
  github.com/uber-go/tally/v4.(*scope).reportRegistry()
      /Users/vytenis/go/tally/scope.go:278 +0xc4
  github.com/uber-go/tally/v4.(*scope).reportLoopRun()
      /Users/vytenis/go/tally/scope.go:270 +0x58
  github.com/uber-go/tally/v4.TestCachedReporterInternalMetricsConcurrent.func3()
      /Users/vytenis/go/tally/scope_registry_test.go:316 +0x134

Goroutine 8 (running) created at:
  github.com/uber-go/tally/v4.TestCachedReporterInternalMetricsConcurrent()
      /Users/vytenis/go/tally/scope_registry_test.go:280 +0x3cc
  testing.tRunner()
      /opt/homebrew/Cellar/go@1.21/1.21.9/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go@1.21/1.21.9/libexec/src/testing/testing.go:1648 +0x40

Goroutine 9 (running) created at:
  github.com/uber-go/tally/v4.TestCachedReporterInternalMetricsConcurrent()
      /Users/vytenis/go/tally/scope_registry_test.go:302 +0x4f0
  testing.tRunner()
      /opt/homebrew/Cellar/go@1.21/1.21.9/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go@1.21/1.21.9/libexec/src/testing/testing.go:1648 +0x40
==================

Copy link
Collaborator

@abliqo abliqo left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@vdarulis vdarulis merged commit 22fe011 into master Apr 12, 2024
5 checks passed
dnr added a commit to temporalio/temporal that referenced this pull request May 4, 2024
## What changed?
Include uber-go/tally#257.
After a release that includes that commit we can switch back to a
released version.

## Why?
Fix race condition that causes flaky tests.

## How did you test it?
existing tests
@vdarulis vdarulis deleted the v/fix_race branch June 25, 2024 19:24
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.

2 participants