Skip to content

fix(storage): Close meter provider after client close#14171

Open
krishnamd-jkp wants to merge 1 commit intogoogleapis:mainfrom
krishnamd-jkp:fix_meter_provider
Open

fix(storage): Close meter provider after client close#14171
krishnamd-jkp wants to merge 1 commit intogoogleapis:mainfrom
krishnamd-jkp:fix_meter_provider

Conversation

@krishnamd-jkp
Copy link
Copy Markdown
Contributor

No description provided.

@krishnamd-jkp krishnamd-jkp requested review from a team as code owners March 13, 2026 06:36
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Mar 13, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces two positive changes: it corrects the shutdown order of the gRPC connection and the metrics provider to prevent lost metrics, and it avoids shutting down a user-provided meter provider, which is proper resource lifecycle management. My review focuses on a latent bug in the metrics shutdown logic where an expired context is used, which would cause shutdown to fail. I've also pointed out that shutdown errors are not being handled. I've provided suggestions to fix these issues by using a new context for the shutdown operation and propagating any errors to the caller.

// true if the provider was created by the SDK and should be shut down here
isDefaultProvider bool
// clean func to call when closing gRPC client
close func()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The close function should return an error to propagate the result of provider.Shutdown. The Shutdown method on MeterProvider can return an error (e.g., if the shutdown context times out) which should be handled by the client's Close method.

Suggested change
close func()
close func() error

Comment on lines 214 to 218
close: func() {
provider.Shutdown(ctx)
if isDefault {
provider.Shutdown(ctx)
}
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There are two issues with the current implementation of close:

  1. The function captures ctx from newGRPCMetricContext. This context is associated with client initialization and is likely to be expired when client.Close() is called. This would cause provider.Shutdown(ctx) to fail. A new context should be created for the shutdown process.
  2. The error returned by provider.Shutdown is ignored. This error should be returned to the caller.

Note that after this change, grpcStorageClient.Close() will also need to be updated to handle the error returned from metricsContext.close().

		close: func() error {
			if isDefault {
				// Use a background context with a timeout for shutdown to ensure
				// it can complete even if the original context is expired.
				shutdownCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
				defer cancel()
				return provider.Shutdown(shutdownCtx)
			}
			return nil
		},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant