Missing lock around rng access in rand.go #5814
Comments
pkwarren
added a commit
to pkwarren/opentelemetry-go
that referenced
this issue
Sep 12, 2024
Update rand.Rand usage to add locking around a global rand.Rand instance, which is not safe for concurrent access by multiple goroutines. Fixes open-telemetry#5814.
pkwarren
added a commit
to pkwarren/opentelemetry-go
that referenced
this issue
Sep 12, 2024
Update rand.Rand usage to add locking around a global rand.Rand instance, which is not safe for concurrent access by multiple goroutines. Fixes open-telemetry#5814.
|
For those also looking into this, this looks to be cause by an |
|
Reproduction: package metric // import "go.opentelemetry.io/otel/sdk/metric"
import (
"context"
"sync"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/otel/sdk/metric/metricdata"
)
func TestRandomExemplarConcurrentSafe(t *testing.T) {
t.Setenv("OTEL_METRICS_EXEMPLAR_FILTER", "always_on")
r := NewManualReader()
p := NewMeterProvider(WithReader(r))
m := p.Meter("exemplar-concurrency")
// Use two instruments to get concurrent access to any shared globals.
i0, err := m.Int64Counter("counter.0")
require.NoError(t, err)
i1, err := m.Int64Counter("counter.1")
require.NoError(t, err)
ctx := context.Background()
const goRoutines = 10
var wg sync.WaitGroup
wg.Add(goRoutines)
done := make(chan struct{})
go func() {
wg.Wait()
close(done)
}()
go func() {
select {
case <-done:
return
default:
}
var out metricdata.ResourceMetrics
require.NotPanics(t, func() {
require.NoError(t, r.Collect(ctx, &out))
})
}()
for n := 0; n < goRoutines; n++ {
go func() {
defer wg.Done()
require.NotPanics(t, func() {
i0.Add(ctx, 1)
i1.Add(ctx, 2)
})
}()
}
<-done
var out metricdata.ResourceMetrics
require.NoError(t, r.Collect(ctx, &out))
p.Shutdown(ctx)
}The key here is the duplicate instruments (i.e. This does not fail reliably with only one test run. Need to build multiple collections into this test to cause it to do so. |
MrAlias
added a commit
to MrAlias/opentelemetry-go
that referenced
this issue
Sep 13, 2024
This verifies this as a fix to open-telemetry#5814.
dashpole
added a commit
that referenced
this issue
Oct 11, 2024
### Added - Add `go.opentelemetry.io/otel/sdk/metric/exemplar` package which includes `Exemplar`, `Filter`, `TraceBasedFilter`, `AlwaysOnFilter`, `HistogramReservoir`, `FixedSizeReservoir`, `Reservoir`, `Value` and `ValueType` types. These will be used for configuring the exemplar reservoir for the metrics sdk. (#5747, #5862) - Add `WithExportBufferSize` option to log batch processor.(#5877) ### Changed - Enable exemplars by default in `go.opentelemetry.io/otel/sdk/metric`. Exemplars can be disabled by setting `OTEL_METRICS_EXEMPLAR_FILTER=always_off` (#5778) - `Logger.Enabled` in `go.opentelemetry.io/otel/log` now accepts a newly introduced `EnabledParameters` type instead of `Record`. (#5791) - `FilterProcessor.Enabled` in `go.opentelemetry.io/otel/sdk/log/internal/x` now accepts `EnabledParameters` instead of `Record`. (#5791) - The `Record` type in `go.opentelemetry.io/otel/log` is no longer comparable. (#5847) - Performance improvements for the trace SDK `SetAttributes` method in `Span`. (#5864) - Reduce memory allocations for the `Event` and `Link` lists in `Span`. (#5858) - Performance improvements for the trace SDK `AddEvent`, `AddLink`, `RecordError` and `End` methods in `Span`. (#5874) ### Deprecated - Deprecate all examples under `go.opentelemetry.io/otel/example` as they are moved to [Contrib repository](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/examples). (#5854) ### Fixed - The race condition for multiple `FixedSize` exemplar reservoirs identified in #5814 is resolved. (#5819) - Fix log records duplication in case of heterogeneous resource attributes by correctly mapping each log record to it's resource and scope. (#5803) - Fix timer channel drain to avoid hanging on Go 1.23. (#5868) - Fix delegation for global meter providers, and panic when calling otel.SetMeterProvider. (#5827) - Change the `reflect.TypeOf` to use a nil pointer to not allocate on the heap unless necessary. (#5827)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
A rand.Rand instance is used without locking in rand.go, leading to potential panics.
Environment
Steps To Reproduce
Int64Histogram.Recordfrom multiple goroutines.Expected behavior
No panics from code with concurrent calls to a histogram.
The text was updated successfully, but these errors were encountered: