From baf47cd60e0a44ffa7f8e3b8b542f4cdedb2b945 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 31 Aug 2022 18:07:38 -0400 Subject: [PATCH 1/5] Added flush and cleanup to End(), also fixing tests --- gcp/observability/observability.go | 19 +++++++++++++++++++ gcp/observability/observability_test.go | 7 ------- gcp/observability/opencensus.go | 5 ++++- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/gcp/observability/observability.go b/gcp/observability/observability.go index 1ab20d4cab5..aed604dbbbd 100644 --- a/gcp/observability/observability.go +++ b/gcp/observability/observability.go @@ -29,7 +29,12 @@ import ( "context" "fmt" + "contrib.go.opencensus.io/exporter/stackdriver" + + "go.opencensus.io/stats/view" + "go.opencensus.io/trace" "google.golang.org/grpc/grpclog" + "google.golang.org/grpc/internal" ) var logger = grpclog.Component("observability") @@ -80,4 +85,18 @@ func Start(ctx context.Context) error { // Note: this method should only be invoked once. func End() { defaultLogger.Close() + if exporter != nil { + if sdExporter, ok := exporter.(*stackdriver.Exporter); ok { + sdExporter.Flush() + sdExporter.Close() + } + + // Call these unconditionally, doesn't matter if not registered, will be + // a noop if not registered. + trace.UnregisterExporter(exporter) + view.UnregisterExporter(exporter) + + internal.ClearExtraDialOptions() + internal.ClearExtraServerOptions() + } } diff --git a/gcp/observability/observability_test.go b/gcp/observability/observability_test.go index 3e8f1d704bd..120022bc10b 100644 --- a/gcp/observability/observability_test.go +++ b/gcp/observability/observability_test.go @@ -35,7 +35,6 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" grpclogrecordpb "google.golang.org/grpc/gcp/observability/internal/logging" - "google.golang.org/grpc/internal" iblog "google.golang.org/grpc/internal/binarylog" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/leakcheck" @@ -875,12 +874,6 @@ func (s) TestCustomTagsTracingMetrics(t *testing.T) { cleanup, err := createTmpConfigInFileSystem(configJSON) defer cleanup() - // To clear globally registered tracing and metrics exporters. - defer func() { - internal.ClearExtraDialOptions() - internal.ClearExtraServerOptions() - }() - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() err = Start(ctx) diff --git a/gcp/observability/opencensus.go b/gcp/observability/opencensus.go index 7d297f90bfc..6d9230cfff4 100644 --- a/gcp/observability/opencensus.go +++ b/gcp/observability/opencensus.go @@ -58,6 +58,8 @@ type tracingMetricsExporter interface { view.Exporter } +var exporter tracingMetricsExporter + // global to stub out in tests var newExporter = newStackdriverExporter @@ -87,7 +89,8 @@ func startOpenCensus(config *config) error { return nil } - exporter, err := newExporter(config) + var err error + exporter, err = newExporter(config) if err != nil { return err } From 16d2b03b421b830eaad72348d0f168fecbafc37b Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 31 Aug 2022 19:28:02 -0400 Subject: [PATCH 2/5] Add cpu 1,4 flag to testing.yml --- .github/workflows/testing.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml index 84c2907bbb6..6d9571707bf 100644 --- a/.github/workflows/testing.yml +++ b/.github/workflows/testing.yml @@ -109,7 +109,7 @@ jobs: cd "${GITHUB_WORKSPACE}" for MOD_FILE in $(find . -name 'go.mod' | grep -Ev '^\./go\.mod'); do pushd "$(dirname ${MOD_FILE})" - go test ${{ matrix.testflags }} -timeout 2m ./... + go test ${{ matrix.testflags }} -cpu 1,4 -timeout 2m ./... popd done From d6f177fb84dace2999fa641042f04483bde9c254 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 1 Sep 2022 18:26:05 -0400 Subject: [PATCH 3/5] Responded to Doug's comment --- gcp/observability/observability.go | 20 +------------------- gcp/observability/opencensus.go | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/gcp/observability/observability.go b/gcp/observability/observability.go index aed604dbbbd..3855bc7ebaf 100644 --- a/gcp/observability/observability.go +++ b/gcp/observability/observability.go @@ -29,12 +29,7 @@ import ( "context" "fmt" - "contrib.go.opencensus.io/exporter/stackdriver" - - "go.opencensus.io/stats/view" - "go.opencensus.io/trace" "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/internal" ) var logger = grpclog.Component("observability") @@ -85,18 +80,5 @@ func Start(ctx context.Context) error { // Note: this method should only be invoked once. func End() { defaultLogger.Close() - if exporter != nil { - if sdExporter, ok := exporter.(*stackdriver.Exporter); ok { - sdExporter.Flush() - sdExporter.Close() - } - - // Call these unconditionally, doesn't matter if not registered, will be - // a noop if not registered. - trace.UnregisterExporter(exporter) - view.UnregisterExporter(exporter) - - internal.ClearExtraDialOptions() - internal.ClearExtraServerOptions() - } + stopOpenCensus() } diff --git a/gcp/observability/opencensus.go b/gcp/observability/opencensus.go index 6d9230cfff4..bc58493fa27 100644 --- a/gcp/observability/opencensus.go +++ b/gcp/observability/opencensus.go @@ -121,3 +121,22 @@ func startOpenCensus(config *config) error { return nil } + +// stopOpenCensus flushes the exporter's and cleans up globals across all +// packages if exporter was created. +func stopOpenCensus() { + if exporter != nil { + if sdExporter, ok := exporter.(*stackdriver.Exporter); ok { + sdExporter.Flush() + sdExporter.Close() + } + + // Call these unconditionally, doesn't matter if not registered, will be + // a noop if not registered. + trace.UnregisterExporter(exporter) + view.UnregisterExporter(exporter) + + internal.ClearExtraDialOptions() + internal.ClearExtraServerOptions() + } +} From ca8120c2fcdee83d9a12db95065f9da96a6dd51a Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 6 Sep 2022 14:13:57 -0400 Subject: [PATCH 4/5] Responded to Doug's comments --- gcp/observability/opencensus.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gcp/observability/opencensus.go b/gcp/observability/opencensus.go index bc58493fa27..2c6091cb580 100644 --- a/gcp/observability/opencensus.go +++ b/gcp/observability/opencensus.go @@ -126,17 +126,17 @@ func startOpenCensus(config *config) error { // packages if exporter was created. func stopOpenCensus() { if exporter != nil { - if sdExporter, ok := exporter.(*stackdriver.Exporter); ok { - sdExporter.Flush() - sdExporter.Close() - } + internal.ClearExtraDialOptions() + internal.ClearExtraServerOptions() // Call these unconditionally, doesn't matter if not registered, will be // a noop if not registered. trace.UnregisterExporter(exporter) view.UnregisterExporter(exporter) - internal.ClearExtraDialOptions() - internal.ClearExtraServerOptions() + if sdExporter, ok := exporter.(*stackdriver.Exporter); ok { + sdExporter.Flush() + sdExporter.Close() + } } } From fff3e5c3df0bfc0f5461745c57e58e9d10b5c9ed Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 6 Sep 2022 19:08:25 -0400 Subject: [PATCH 5/5] Responded to Doug's comment --- gcp/observability/observability_test.go | 6 ++++++ gcp/observability/opencensus.go | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/gcp/observability/observability_test.go b/gcp/observability/observability_test.go index 120022bc10b..e9238f898ab 100644 --- a/gcp/observability/observability_test.go +++ b/gcp/observability/observability_test.go @@ -386,6 +386,12 @@ func (fe *fakeOpenCensusExporter) ExportSpan(vd *trace.SpanData) { fe.t.Logf("Span[%v]", vd.Name) } +func (fe *fakeOpenCensusExporter) Flush() {} + +func (fe *fakeOpenCensusExporter) Close() error { + return nil +} + func (s) TestLoggingForOkCall(t *testing.T) { te := newTest(t) defer te.tearDown() diff --git a/gcp/observability/opencensus.go b/gcp/observability/opencensus.go index 2c6091cb580..ccaa6a98a42 100644 --- a/gcp/observability/opencensus.go +++ b/gcp/observability/opencensus.go @@ -56,6 +56,8 @@ func tagsToTraceAttributes(tags map[string]string) map[string]interface{} { type tracingMetricsExporter interface { trace.Exporter view.Exporter + Flush() + Close() error } var exporter tracingMetricsExporter @@ -134,9 +136,7 @@ func stopOpenCensus() { trace.UnregisterExporter(exporter) view.UnregisterExporter(exporter) - if sdExporter, ok := exporter.(*stackdriver.Exporter); ok { - sdExporter.Flush() - sdExporter.Close() - } + exporter.Flush() + exporter.Close() } }