From 236acca1974a910cc1d04206a8522a244504dc14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 27 Jul 2023 09:26:55 +0200 Subject: [PATCH 1/5] Remove Reader.ForceFlush and ManualReader.ForceFlush --- CHANGELOG.md | 4 ++++ sdk/metric/config.go | 4 +++- sdk/metric/manual_reader.go | 7 ------- sdk/metric/periodic_reader_test.go | 12 +++++++++++- sdk/metric/reader.go | 10 ---------- sdk/metric/reader_test.go | 20 +++++++------------- 6 files changed, 25 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 691ed9e8c4f..c2138bad6e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Restrict `Meter`s in `go.opentelemetry.io/otel/sdk/metric` to only register and collect instruments it created. (#4333) - `PeriodicReader.Shutdown` in `go.opentelemetry.io/otel/sdk/metric` now applies the periodic reader's timeout by default. (#4356) +### Removed + +- Remove `Reader.ForceFlush` in `go.opentelemetry.io/otel/metric`. Notice that `PeriodicReader.ForceFlush` is still available. (#TODO) + ### Fixed - Correctly format log messages from the `go.opentelemetry.io/otel/exporters/zipkin` exporter. (#4143) diff --git a/sdk/metric/config.go b/sdk/metric/config.go index c837df8b76f..0b191128494 100644 --- a/sdk/metric/config.go +++ b/sdk/metric/config.go @@ -37,7 +37,9 @@ func (c config) readerSignals() (forceFlush, shutdown func(context.Context) erro var fFuncs, sFuncs []func(context.Context) error for _, r := range c.readers { sFuncs = append(sFuncs, r.Shutdown) - fFuncs = append(fFuncs, r.ForceFlush) + if f, ok := r.(interface{ ForceFlush(context.Context) error }); ok { + fFuncs = append(fFuncs, f.ForceFlush) + } } return unify(fFuncs), unifyShutdown(sFuncs) diff --git a/sdk/metric/manual_reader.go b/sdk/metric/manual_reader.go index 898af86edc0..a7715f5b34f 100644 --- a/sdk/metric/manual_reader.go +++ b/sdk/metric/manual_reader.go @@ -91,13 +91,6 @@ func (mr *ManualReader) aggregation(kind InstrumentKind) aggregation.Aggregation return mr.aggregationSelector(kind) } -// ForceFlush is a no-op, it always returns nil. -// -// This method is safe to call concurrently. -func (mr *ManualReader) ForceFlush(context.Context) error { - return nil -} - // Shutdown closes any connections and frees any resources used by the reader. // // This method is safe to call concurrently. diff --git a/sdk/metric/periodic_reader_test.go b/sdk/metric/periodic_reader_test.go index e5b279ec238..a3e3dccae8e 100644 --- a/sdk/metric/periodic_reader_test.go +++ b/sdk/metric/periodic_reader_test.go @@ -20,6 +20,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "go.opentelemetry.io/otel" @@ -198,7 +199,7 @@ func (e *fnExporter) Shutdown(ctx context.Context) error { type periodicReaderTestSuite struct { *readerTestSuite - ErrReader Reader + ErrReader *PeriodicReader } func (ts *periodicReaderTestSuite) SetupTest() { @@ -379,6 +380,15 @@ func TestPeriodicReaderFlushesPending(t *testing.T) { }) } +func (ts *readerTestSuite) TestPeriodicReaderMultipleForceFlush(t *testing.T) { + ctx := context.Background() + r := NewPeriodicReader(new(fnExporter)) + r.register(testSDKProducer{}) + r.RegisterProducer(testExternalProducer{}) + require.NoError(t, r.ForceFlush(ctx)) + require.NoError(t, r.ForceFlush(ctx)) +} + func BenchmarkPeriodicReader(b *testing.B) { b.Run("Collect", benchReaderCollectFunc( NewPeriodicReader(new(fnExporter)), diff --git a/sdk/metric/reader.go b/sdk/metric/reader.go index a281104bd08..6bcbaa41a47 100644 --- a/sdk/metric/reader.go +++ b/sdk/metric/reader.go @@ -78,16 +78,6 @@ type Reader interface { // passed context is expected to be honored. Collect(ctx context.Context, rm *metricdata.ResourceMetrics) error - // ForceFlush flushes all metric measurements held in an export pipeline. - // - // This deadline or cancellation of the passed context are honored. An appropriate - // error will be returned in these situations. There is no guaranteed that all - // telemetry be flushed or all resources have been released in these - // situations. - // - // This method needs to be concurrent safe. - ForceFlush(context.Context) error - // Shutdown flushes all metric measurements held in an export pipeline and releases any // held computational resources. // diff --git a/sdk/metric/reader_test.go b/sdk/metric/reader_test.go index d375a1b4f6c..27939f21fbc 100644 --- a/sdk/metric/reader_test.go +++ b/sdk/metric/reader_test.go @@ -93,14 +93,6 @@ func (ts *readerTestSuite) TestShutdownTwice() { ts.ErrorIs(ts.Reader.Shutdown(ctx), ErrReaderShutdown) } -func (ts *readerTestSuite) TestMultipleForceFlush() { - ctx := context.Background() - ts.Reader.register(testSDKProducer{}) - ts.Reader.RegisterProducer(testExternalProducer{}) - ts.Require().NoError(ts.Reader.ForceFlush(ctx)) - ts.NoError(ts.Reader.ForceFlush(ctx)) -} - func (ts *readerTestSuite) TestMultipleRegister() { p0 := testSDKProducer{ produceFunc: func(ctx context.Context, rm *metricdata.ResourceMetrics) error { @@ -174,11 +166,13 @@ func (ts *readerTestSuite) TestMethodConcurrentSafe() { _ = ts.Reader.Collect(ctx, nil) }() - wg.Add(1) - go func() { - defer wg.Done() - _ = ts.Reader.ForceFlush(ctx) - }() + if f, ok := ts.Reader.(interface{ ForceFlush(context.Context) error }); ok { + wg.Add(1) + go func() { + defer wg.Done() + _ = f.ForceFlush(ctx) + }() + } wg.Add(1) go func() { From 206e386b676f675b2dbf3f6a2d4441743fd99350 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 27 Jul 2023 09:38:31 +0200 Subject: [PATCH 2/5] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2138bad6e6..2c898d17d3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Removed -- Remove `Reader.ForceFlush` in `go.opentelemetry.io/otel/metric`. Notice that `PeriodicReader.ForceFlush` is still available. (#TODO) +- Remove `Reader.ForceFlush` in `go.opentelemetry.io/otel/metric`. Notice that `PeriodicReader.ForceFlush` is still available. (#4375) ### Fixed From abf6fbfb219bce1dbb096000bea6fd5992fff4f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 27 Jul 2023 09:39:58 +0200 Subject: [PATCH 3/5] Fix TestPeriodicReaderMultipleForceFlush --- sdk/metric/periodic_reader_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/metric/periodic_reader_test.go b/sdk/metric/periodic_reader_test.go index a3e3dccae8e..bb9210159e1 100644 --- a/sdk/metric/periodic_reader_test.go +++ b/sdk/metric/periodic_reader_test.go @@ -380,7 +380,7 @@ func TestPeriodicReaderFlushesPending(t *testing.T) { }) } -func (ts *readerTestSuite) TestPeriodicReaderMultipleForceFlush(t *testing.T) { +func TestPeriodicReaderMultipleForceFlush(t *testing.T) { ctx := context.Background() r := NewPeriodicReader(new(fnExporter)) r.register(testSDKProducer{}) From 997220642f03f065a205bd5393ae2898be818671 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 9 Aug 2023 09:09:27 +0200 Subject: [PATCH 4/5] Document implicit Reader.ForceFlush in MeterProvider.ForceFlush --- sdk/metric/provider.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sdk/metric/provider.go b/sdk/metric/provider.go index 49dd071c9d3..7d1a9183ce1 100644 --- a/sdk/metric/provider.go +++ b/sdk/metric/provider.go @@ -110,6 +110,9 @@ func (mp *MeterProvider) Meter(name string, options ...metric.MeterOption) metri // telemetry be flushed or all resources have been released in these // situations. // +// ForceFlush calls ForceFlush(context.Context) error +// on all Readers that implements this method. +// // This method is safe to call concurrently. func (mp *MeterProvider) ForceFlush(ctx context.Context) error { if mp.forceFlush != nil { From add27174f9ccd5c9723bb97d995794f471a7dbca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 11 Aug 2023 09:02:21 +0200 Subject: [PATCH 5/5] Update CHANGELOG.md Co-authored-by: Tyler Yahn --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c23705ea17..61a06cf2cad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,7 +47,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Removed -- Remove `Reader.ForceFlush` in `go.opentelemetry.io/otel/metric`. Notice that `PeriodicReader.ForceFlush` is still available. (#4375) +- Remove `Reader.ForceFlush` in `go.opentelemetry.io/otel/metric`. + Notice that `PeriodicReader.ForceFlush` is still available. (#4375) ### Fixed