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

Use RWLock in prometheus metrics #331

Merged
merged 2 commits into from
Oct 6, 2021
Merged

Use RWLock in prometheus metrics #331

merged 2 commits into from
Oct 6, 2021

Conversation

MikeGoldsmith
Copy link
Contributor

Which problem is this PR solving?

A mutex is used in the prometheus metrics implementation and in #324 was used the lock when using the different metrics types. This PR switches the mutex type to a RWLock to allow concurrent retrieving of metrics from the map.

Register is the only place where a write lock is required.

Short description of the changes

  • Update mutex to RWLock
  • use RLock/RUnlock when using the different metric types

@MikeGoldsmith MikeGoldsmith added type: enhancement New feature or request version: bump patch A PR with release-worthy changes and is backwards-compatible. labels Oct 4, 2021
@MikeGoldsmith MikeGoldsmith self-assigned this Oct 4, 2021
@MikeGoldsmith MikeGoldsmith requested a review from a team October 4, 2021 15:18
Copy link
Member

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

During our team sync today, we talked of adding tests around concurrency and race conditions for this change.

metrics/prometheus.go Show resolved Hide resolved
@robbkidd robbkidd self-assigned this Oct 5, 2021
@robbkidd
Copy link
Member

robbkidd commented Oct 5, 2021

The new TestRaciness test shows the race condition when run against v1.5.0, the version prior to Lock or RWLock being added to the other PromMetrics functions.

go test -tags race --race --timeout 60s -v ./...

=== RUN   TestMultipleRegistrations
--- PASS: TestMultipleRegistrations (0.00s)
=== RUN   TestRaciness
==================
WARNING: DATA RACE
Write at 0x00c0002d3e30 by goroutine 19:
  runtime.mapassign_faststr()
      /Users/robbkidd/.asdf/installs/golang/1.16.6/go/src/runtime/map_faststr.go:202 +0x0
  github.com/honeycombio/refinery/metrics.(*PromMetrics).Register()
      /Users/robbkidd/work/sampling/refinery/metrics/prometheus.go:78 +0x1cc
  github.com/honeycombio/refinery/metrics.TestRaciness.func1()
      /Users/robbkidd/work/sampling/refinery/metrics/prometheus_test.go:46 +0xc6

Previous read at 0x00c0002d3e30 by goroutine 20:
  runtime.mapaccess2_faststr()
      /Users/robbkidd/.asdf/installs/golang/1.16.6/go/src/runtime/map_faststr.go:107 +0x0
  github.com/honeycombio/refinery/metrics.(*PromMetrics).Increment()
      /Users/robbkidd/work/sampling/refinery/metrics/prometheus.go:82 +0x76
  github.com/honeycombio/refinery/metrics.TestRaciness.func2()
      /Users/robbkidd/work/sampling/refinery/metrics/prometheus_test.go:50 +0x4d

Goroutine 19 (running) created at:
  github.com/honeycombio/refinery/metrics.TestRaciness()
      /Users/robbkidd/work/sampling/refinery/metrics/prometheus_test.go:44 +0x23e
  testing.tRunner()
      /Users/robbkidd/.asdf/installs/golang/1.16.6/go/src/testing/testing.go:1193 +0x202

Goroutine 20 (finished) created at:
  github.com/honeycombio/refinery/metrics.TestRaciness()
      /Users/robbkidd/work/sampling/refinery/metrics/prometheus_test.go:49 +0x26a
  testing.tRunner()
      /Users/robbkidd/.asdf/installs/golang/1.16.6/go/src/testing/testing.go:1193 +0x202
==================
    testing.go:1092: race detected during execution of test
--- FAIL: TestRaciness (0.01s)
=== CONT  
    testing.go:1092: race detected during execution of test
FAIL
FAIL    github.com/honeycombio/refinery/metrics 0.234s

@robbkidd robbkidd dismissed their stale review October 5, 2021 20:49

I contributed the test I requested.


go func(j int) {
p.Increment("race")
}(i)
Copy link
Member

@robbkidd robbkidd Oct 5, 2021

Choose a reason for hiding this comment

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

Oh. Heh. A previous iteration of this used p.Count("race", j) which needed i to be passed in to avoid triggering the wrong race condition. Doesn't need to be any more. 😶

@MikeGoldsmith MikeGoldsmith merged commit ba7daaa into main Oct 6, 2021
@MikeGoldsmith MikeGoldsmith deleted the mike/rwlock branch October 6, 2021 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request version: bump patch A PR with release-worthy changes and is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants