Skip to content

fix: remove redundant forceflush from metricmod shutdown #74

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.

Sign up for GitHub

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

Open
juho9000 wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −22
Open

fix: remove redundant forceflush from metricmod shutdown #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions metermod/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package metermod
import (
"cmp"
"context"
"errors"
"fmt"
"os"
"strings"
Expand All @@ -19,10 +18,7 @@ import (

const ID = "metermod"

const (
ErrMissingProvider = errStr("meter provider not set")
ErrFlushFailed = errStr("failed to flush remaining metrics")
)
const ErrMissingProvider = errStr("meter provider not set")

type errStr string

Expand Down Expand Up @@ -66,12 +62,7 @@ func (p *Provider) Run() error {

func (p *Provider) Stop() error {
close(p.done)
flushErr := p.provider.ForceFlush(context.Background())
if flushErr != nil {
flushErr = fmt.Errorf("%w: %w", ErrFlushFailed, flushErr)
}
shutdownErr := p.provider.Shutdown(context.Background())
return errors.Join(flushErr, shutdownErr)
return p.provider.Shutdown(context.Background())
}

func (p *Provider) ID() string { return ID }
Expand Down
27 changes: 16 additions & 11 deletions metermod/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ func TestProvider(t *testing.T) {
opts []metermod.Opt
contentType string
expectCalled bool
err error
}{
{
name: "Default",
Expand All @@ -37,14 +36,6 @@ func TestProvider(t *testing.T) {
contentType: "application/x-protobuf",
expectCalled: true,
},
{
// We dont have proper GRPC server so exporter will retry until it reached OTEL_EXPORTER_OTLP_METRICS_TIMEOUT
name: "WithGRPC",
opts: []metermod.Opt{metermod.WithGRPC()},
contentType: "",
expectCalled: true,
err: metermod.ErrFlushFailed,
},
}

for _, tt := range test {
Expand All @@ -58,7 +49,6 @@ func TestProvider(t *testing.T) {
t.Cleanup(srv.Close)

t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", srv.URL)
t.Setenv("OTEL_EXPORTER_OTLP_METRICS_TIMEOUT", "10") // Faster timeout for GRPC test

p := metermod.New(tt.opts...)
require.NoError(t, p.Init())
Expand All @@ -68,14 +58,29 @@ func TestProvider(t *testing.T) {
_, span := otel.GetTracerProvider().Tracer("test").Start(context.Background(), "test")
span.End()

require.ErrorIs(t, p.Stop(), tt.err)
require.NoError(t, p.Stop())
require.NoError(t, wg.Wait())
require.Equal(t, "metermod", p.ID())
require.Equal(t, tt.expectCalled, called.Load())
})
}
}

// Shutdown does a final flush, and Stop must return any error from it. Pointing the
// exporter at a closed port makes that flush fail.
func TestProvider_Stop_SurfacesExportError(t *testing.T) {
t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "http://127.0.0.1:1")
t.Setenv("OTEL_EXPORTER_OTLP_METRICS_TIMEOUT", "10") // Fail fast instead of retrying.

p := metermod.New(metermod.WithGRPC())
require.NoError(t, p.Init())
wg := &errgroup.ErrGroup{}
wg.Go(p.Run)

require.Error(t, p.Stop())
require.NoError(t, wg.Wait())
}

func TestProvider_WithProvider_nil(t *testing.T) {
p := metermod.New(metermod.WithProvider(nil))
require.ErrorIs(t, p.Init(), metermod.ErrMissingProvider)
Expand Down
Loading