diff --git a/api/core/number_test.go b/api/core/number_test.go index 67583686c54..0fb75b89294 100644 --- a/api/core/number_test.go +++ b/api/core/number_test.go @@ -67,12 +67,14 @@ func TestNumber(t *testing.T) { cmpsForPos := [3]int{1, 1, 0} type testcase struct { - n Number + // n needs to be aligned for 64-bit atomic operations. + n Number + // nums needs to be aligned for 64-bit atomic operations. + nums [3]Number kind NumberKind pos bool zero bool neg bool - nums [3]Number cmps [3]int } testcases := []testcase{ diff --git a/api/metric/alignment_test.go b/api/metric/alignment_test.go new file mode 100644 index 00000000000..92ca9937a0a --- /dev/null +++ b/api/metric/alignment_test.go @@ -0,0 +1,24 @@ +package metric + +import ( + "os" + "testing" + "unsafe" + + ottest "go.opentelemetry.io/otel/internal/testing" +) + +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Measurement.number", + Offset: unsafe.Offsetof(Measurement{}.number), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} diff --git a/api/metric/api.go b/api/metric/api.go index f96bd75d5bc..2e546c697a4 100644 --- a/api/metric/api.go +++ b/api/metric/api.go @@ -85,8 +85,9 @@ type MeasureOptionApplier interface { // values. Instances of this type should be created by instruments // (e.g., Int64Counter.Measurement()). type Measurement struct { - instrument InstrumentImpl + // number needs to be aligned for 64-bit atomic operations. number core.Number + instrument InstrumentImpl } // Instrument returns the instrument that created this measurement. diff --git a/internal/metric/mock.go b/internal/metric/mock.go index 68b7d7582a9..8bd37574f5b 100644 --- a/internal/metric/mock.go +++ b/internal/metric/mock.go @@ -41,9 +41,10 @@ type ( } Batch struct { + // Measurement needs to be aligned for 64-bit atomic operations. + Measurements []Measurement Ctx context.Context LabelSet *LabelSet - Measurements []Measurement } MeterProvider struct { @@ -58,8 +59,9 @@ type ( Kind int8 Measurement struct { - Instrument *Instrument + // Number needs to be aligned for 64-bit atomic operations. Number core.Number + Instrument *Instrument } ) diff --git a/internal/metric/mock_test.go b/internal/metric/mock_test.go new file mode 100644 index 00000000000..7995b17d02d --- /dev/null +++ b/internal/metric/mock_test.go @@ -0,0 +1,28 @@ +package metric + +import ( + "os" + "testing" + "unsafe" + + ottest "go.opentelemetry.io/otel/internal/testing" +) + +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Batch.Measurments", + Offset: unsafe.Offsetof(Batch{}.Measurements), + }, + { + Name: "Measurement.Number", + Offset: unsafe.Offsetof(Measurement{}.Number), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} diff --git a/internal/testing/alignment.go b/internal/testing/alignment.go new file mode 100644 index 00000000000..92b8b1e5834 --- /dev/null +++ b/internal/testing/alignment.go @@ -0,0 +1,57 @@ +package testing + +/* +This file contains common utilities and objects to validate memory alignment +of Go types. The primary use of this functionality is intended to ensure +`struct` fields that need to be 64-bit aligned so they can be passed as +arguments to 64-bit atomic operations. + +The common workflow is to define a slice of `FieldOffset` and pass them to the +`Aligned8Byte` function from within a `TestMain` function from a package's +tests. It is important to make this call from the `TestMain` function prior +to running the rest of the test suit as it can provide useful diagnostics +about field alignment instead of ambiguous nil pointer dereference and runtime +panic. + +For more information: +https://github.com/open-telemetry/opentelemetry-go/issues/341 +*/ + +import ( + "fmt" + "io" +) + +// FieldOffset is a preprocessor representation of a struct field alignment. +type FieldOffset struct { + // Name of the field. + Name string + + // Offset of the field in bytes. + // + // To compute this at compile time use unsafe.Offsetof. + Offset uintptr +} + +// Aligned8Byte returns if all fields are aligned modulo 8-bytes. +// +// Error messaging is printed to out for any fileds determined misaligned. +func Aligned8Byte(fields []FieldOffset, out io.Writer) bool { + misaligned := make([]FieldOffset, 0) + for _, f := range fields { + if f.Offset%8 != 0 { + misaligned = append(misaligned, f) + } + } + + if len(misaligned) == 0 { + return true + } + + fmt.Fprintln(out, "struct fields not aligned for 64-bit atomic operations:") + for _, f := range misaligned { + fmt.Fprintf(out, " %s: %d-byte offset\n", f.Name, f.Offset) + } + + return false +} diff --git a/internal/trace/mock_tracer.go b/internal/trace/mock_tracer.go index d6d033d03be..98d973f8b5b 100644 --- a/internal/trace/mock_tracer.go +++ b/internal/trace/mock_tracer.go @@ -28,12 +28,14 @@ import ( // It only supports ChildOf option. SpanId is atomically increased every time a // new span is created. type MockTracer struct { - // Sampled specifies if the new span should be sampled or not. - Sampled bool - // StartSpanID is used to initialize spanId. It is incremented by one // every time a new span is created. + // + // StartSpanID has to be aligned for 64-bit atomic operations. StartSpanID *uint64 + + // Sampled specifies if the new span should be sampled or not. + Sampled bool } var _ apitrace.Tracer = (*MockTracer)(nil) diff --git a/internal/trace/mock_tracer_test.go b/internal/trace/mock_tracer_test.go new file mode 100644 index 00000000000..fd8791b18ce --- /dev/null +++ b/internal/trace/mock_tracer_test.go @@ -0,0 +1,24 @@ +package trace + +import ( + "os" + "testing" + "unsafe" + + ottest "go.opentelemetry.io/otel/internal/testing" +) + +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "MockTracer.StartSpanID", + Offset: unsafe.Offsetof(MockTracer{}.StartSpanID), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} diff --git a/sdk/metric/aggregator/array/array.go b/sdk/metric/aggregator/array/array.go index f9b79d60df9..3855e8cc28c 100644 --- a/sdk/metric/aggregator/array/array.go +++ b/sdk/metric/aggregator/array/array.go @@ -28,10 +28,11 @@ import ( type ( Aggregator struct { + // ckptSum needs to be aligned for 64-bit atomic operations. + ckptSum core.Number lock sync.Mutex current points checkpoint points - ckptSum core.Number } points []core.Number diff --git a/sdk/metric/aggregator/array/array_test.go b/sdk/metric/aggregator/array/array_test.go index aa5ec525290..53af8415316 100644 --- a/sdk/metric/aggregator/array/array_test.go +++ b/sdk/metric/aggregator/array/array_test.go @@ -18,16 +18,34 @@ import ( "context" "fmt" "math" + "os" "testing" + "unsafe" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregator" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" ) +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Aggregator.ckptSum", + Offset: unsafe.Offsetof(Aggregator{}.ckptSum), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + type updateTest struct { count int absolute bool diff --git a/sdk/metric/aggregator/counter/counter.go b/sdk/metric/aggregator/counter/counter.go index 93660e0d2d9..7d4b8cd0ed4 100644 --- a/sdk/metric/aggregator/counter/counter.go +++ b/sdk/metric/aggregator/counter/counter.go @@ -25,9 +25,11 @@ import ( // Aggregator aggregates counter events. type Aggregator struct { // current holds current increments to this counter record + // current needs to be aligned for 64-bit atomic operations. current core.Number // checkpoint is a temporary used during Checkpoint() + // checkpoint needs to be aligned for 64-bit atomic operations. checkpoint core.Number } diff --git a/sdk/metric/aggregator/counter/counter_test.go b/sdk/metric/aggregator/counter/counter_test.go index 7ce5d16cdac..4c66a78e4a1 100644 --- a/sdk/metric/aggregator/counter/counter_test.go +++ b/sdk/metric/aggregator/counter/counter_test.go @@ -16,17 +16,39 @@ package counter import ( "context" + "os" "testing" + "unsafe" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" ) const count = 100 +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Aggregator.current", + Offset: unsafe.Offsetof(Aggregator{}.current), + }, + { + Name: "Aggregator.checkpoint", + Offset: unsafe.Offsetof(Aggregator{}.checkpoint), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + func TestCounterMonotonic(t *testing.T) { ctx := context.Background() diff --git a/sdk/metric/aggregator/gauge/gauge.go b/sdk/metric/aggregator/gauge/gauge.go index 5b86d7585bf..a9e73f73de8 100644 --- a/sdk/metric/aggregator/gauge/gauge.go +++ b/sdk/metric/aggregator/gauge/gauge.go @@ -45,6 +45,8 @@ type ( // a sequence number to determine the winner of a race. gaugeData struct { // value is the int64- or float64-encoded Set() data + // + // value needs to be aligned for 64-bit atomic operations. value core.Number // timestamp indicates when this record was submitted. diff --git a/sdk/metric/aggregator/gauge/gauge_test.go b/sdk/metric/aggregator/gauge/gauge_test.go index aeff19643f1..34fd7f53da8 100644 --- a/sdk/metric/aggregator/gauge/gauge_test.go +++ b/sdk/metric/aggregator/gauge/gauge_test.go @@ -17,11 +17,14 @@ package gauge import ( "context" "math/rand" + "os" "testing" + "unsafe" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregator" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" @@ -31,6 +34,21 @@ const count = 100 var _ export.Aggregator = &Aggregator{} +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "gaugeData.value", + Offset: unsafe.Offsetof(gaugeData{}.value), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + func TestGaugeNonMonotonic(t *testing.T) { ctx := context.Background() diff --git a/sdk/metric/aggregator/minmaxsumcount/mmsc.go b/sdk/metric/aggregator/minmaxsumcount/mmsc.go index 24876979fa2..8d9b67c7d6a 100644 --- a/sdk/metric/aggregator/minmaxsumcount/mmsc.go +++ b/sdk/metric/aggregator/minmaxsumcount/mmsc.go @@ -26,12 +26,15 @@ type ( // Aggregator aggregates measure events, keeping only the max, // sum, and count. Aggregator struct { - current state + // current has to be aligned for 64-bit atomic operations. + current state + // checkpoint has to be aligned for 64-bit atomic operations. checkpoint state kind core.NumberKind } state struct { + // all fields have to be aligned for 64-bit atomic operations. count core.Number sum core.Number min core.Number diff --git a/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go b/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go index fb2142fcd76..21feacd19e1 100644 --- a/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go +++ b/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go @@ -18,11 +18,14 @@ import ( "context" "math" "math/rand" + "os" "testing" + "unsafe" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregator" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" @@ -59,6 +62,41 @@ var ( } ) +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Aggregator.current", + Offset: unsafe.Offsetof(Aggregator{}.current), + }, + { + Name: "Aggregator.checkpoint", + Offset: unsafe.Offsetof(Aggregator{}.checkpoint), + }, + { + Name: "state.count", + Offset: unsafe.Offsetof(state{}.count), + }, + { + Name: "state.sum", + Offset: unsafe.Offsetof(state{}.sum), + }, + { + Name: "state.min", + Offset: unsafe.Offsetof(state{}.min), + }, + { + Name: "state.max", + Offset: unsafe.Offsetof(state{}.max), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + func TestMinMaxSumCountAbsolute(t *testing.T) { test.RunProfiles(t, func(t *testing.T, profile test.Profile) { minMaxSumCount(t, profile, positiveOnly) diff --git a/sdk/metric/aggregator/test/test.go b/sdk/metric/aggregator/test/test.go index 3bd484c813f..582183fc189 100644 --- a/sdk/metric/aggregator/test/test.go +++ b/sdk/metric/aggregator/test/test.go @@ -17,10 +17,13 @@ package test import ( "context" "math/rand" + "os" "sort" "testing" + "unsafe" "go.opentelemetry.io/otel/api/core" + ottest "go.opentelemetry.io/otel/internal/testing" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregator" ) @@ -63,9 +66,25 @@ func RunProfiles(t *testing.T, f func(*testing.T, Profile)) { } } +// Ensure local struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "Numbers.numbers", + Offset: unsafe.Offsetof(Numbers{}.numbers), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} + type Numbers struct { - kind core.NumberKind + // numbers has to be aligned for 64-bit atomic operations. numbers []core.Number + kind core.NumberKind } func NewNumbers(kind core.NumberKind) Numbers { diff --git a/sdk/metric/alignment_test.go b/sdk/metric/alignment_test.go new file mode 100644 index 00000000000..9e00ae9e827 --- /dev/null +++ b/sdk/metric/alignment_test.go @@ -0,0 +1,36 @@ +package metric + +import ( + "os" + "testing" + "unsafe" + + ottest "go.opentelemetry.io/otel/internal/testing" +) + +// Ensure struct alignment prior to running tests. +func TestMain(m *testing.M) { + fields := []ottest.FieldOffset{ + { + Name: "record.refcount", + Offset: unsafe.Offsetof(record{}.refcount), + }, + { + Name: "record.collectedEpoch", + Offset: unsafe.Offsetof(record{}.collectedEpoch), + }, + { + Name: "record.modifiedEpoch", + Offset: unsafe.Offsetof(record{}.modifiedEpoch), + }, + { + Name: "record.reclaim", + Offset: unsafe.Offsetof(record{}.reclaim), + }, + } + if !ottest.Aligned8Byte(fields, os.Stderr) { + os.Exit(1) + } + + os.Exit(m.Run()) +} diff --git a/sdk/metric/monotone_test.go b/sdk/metric/monotone_test.go index 0c3c5dadd88..9fb6ee99fcb 100644 --- a/sdk/metric/monotone_test.go +++ b/sdk/metric/monotone_test.go @@ -31,11 +31,12 @@ import ( ) type monotoneBatcher struct { - t *testing.T - - collections int + // currentValue needs to be aligned for 64-bit atomic operations. currentValue *core.Number + collections int currentTime *time.Time + + t *testing.T } func (*monotoneBatcher) AggregatorFor(*export.Descriptor) export.Aggregator { diff --git a/sdk/metric/sdk.go b/sdk/metric/sdk.go index 30cb13e7901..5c15eca14bc 100644 --- a/sdk/metric/sdk.go +++ b/sdk/metric/sdk.go @@ -97,31 +97,39 @@ type ( // `record` in existence at a time, although at most one can // be referenced from the `SDK.current` map. record struct { - // labels is the LabelSet passed by the user. - labels *labels - - // descriptor describes the metric instrument. - descriptor *export.Descriptor - // refcount counts the number of active handles on // referring to this record. active handles prevent // removing the record from the current map. + // + // refcount has to be aligned for 64-bit atomic operations. refcount int64 // collectedEpoch is the epoch number for which this // record has been exported. This is modified by the // `Collect()` method. + // + // collectedEpoch has to be aligned for 64-bit atomic operations. collectedEpoch int64 // modifiedEpoch is the latest epoch number for which // this record was updated. Generally, if // modifiedEpoch is less than collectedEpoch, this // record is due for reclaimation. + // + // modifiedEpoch has to be aligned for 64-bit atomic operations. modifiedEpoch int64 // reclaim is an atomic to control the start of reclaiming. + // + // reclaim has to be aligned for 64-bit atomic operations. reclaim int64 + // labels is the LabelSet passed by the user. + labels *labels + + // descriptor describes the metric instrument. + descriptor *export.Descriptor + // recorder implements the actual RecordOne() API, // depending on the type of aggregation. If nil, the // metric was disabled by the exporter. diff --git a/sdk/metric/stress_test.go b/sdk/metric/stress_test.go index 6e17802dc28..a7149f81068 100644 --- a/sdk/metric/stress_test.go +++ b/sdk/metric/stress_test.go @@ -51,6 +51,7 @@ const ( type ( testFixture struct { + // stop has to be aligned for 64-bit atomic operations. stop int64 expected sync.Map received sync.Map // Note: doesn't require synchronization @@ -93,6 +94,7 @@ type ( // where a race condition causes duplicate records. We always // take the later timestamp. gaugeState struct { + // raw has to be aligned for 64-bit atomic operations. raw core.Number ts time.Time }