Skip to content

Commit 27711f0

Browse files
authored
Ensure we always use logging methods with Context (#225)
* Ensure we always use logging methods with Context * Pass background contexts where we don't accept one from the user May consider changing the API in a future version.
1 parent cf88003 commit 27711f0

File tree

6 files changed

+26
-17
lines changed

6 files changed

+26
-17
lines changed

modal-go/.golangci.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ enable = [
55
"errcheck",
66
"govet",
77
"ineffassign",
8+
"sloglint",
89
"staticcheck",
910
"unused",
1011
"usetesting",
@@ -13,5 +14,8 @@ enable = [
1314
[linters.settings.staticcheck]
1415
checks = ["all"]
1516

17+
[linters.settings.sloglint]
18+
context = "all"
19+
1620
[formatters]
1721
enable = ["gofmt", "goimports"]

modal-go/app.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,6 @@ func (s *appServiceImpl) FromName(ctx context.Context, name string, params *AppF
8282
return nil, err
8383
}
8484

85-
s.client.logger.Debug("Retrieved App", "app_id", resp.GetAppId(), "app_name", name)
85+
s.client.logger.DebugContext(ctx, "Retrieved App", "app_id", resp.GetAppId(), "app_name", name)
8686
return &App{AppID: resp.GetAppId(), Name: name}, nil
8787
}

modal-go/auth_token_manager.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func (m *AuthTokenManager) backgroundRefresh(ctx context.Context) {
108108

109109
// Refresh the token
110110
if _, err := m.FetchToken(ctx); err != nil {
111-
m.logger.Error("Failed to refresh auth token", "error", err)
111+
m.logger.ErrorContext(ctx, "Failed to refresh auth token", "error", err)
112112
// Sleep for 5 seconds before trying again on failure
113113
select {
114114
case <-ctx.Done():
@@ -135,7 +135,7 @@ func (m *AuthTokenManager) FetchToken(ctx context.Context) (string, error) {
135135
if exp := m.decodeJWT(token); exp > 0 {
136136
expiry = exp
137137
} else {
138-
m.logger.Warn("x-modal-auth-token does not contain exp field")
138+
m.logger.WarnContext(ctx, "x-modal-auth-token does not contain exp field")
139139
// We'll use the token, and set the expiry to 20 min from now.
140140
expiry = time.Now().Unix() + DefaultExpiryOffset
141141
}

modal-go/client.go

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ type ClientParams struct {
112112

113113
// NewClientWithOptions generates a new client and allows overriding options in the default profile configuration.
114114
func NewClientWithOptions(params *ClientParams) (*Client, error) {
115+
ctx := context.Background()
115116
if params == nil {
116117
params = &ClientParams{}
117118
}
@@ -161,15 +162,15 @@ func NewClientWithOptions(params *ClientParams) (*Client, error) {
161162
additionalStreamInterceptors: params.GRPCStreamInterceptors,
162163
}
163164

164-
logger.Debug("Initializing Modal client", "version", sdkVersion(), "server_url", profile.ServerURL)
165+
logger.DebugContext(ctx, "Initializing Modal client", "version", sdkVersion(), "server_url", profile.ServerURL)
165166

166167
if params.ControlPlaneClient != nil {
167168
c.cpClient = &clientWithConn{
168169
ModalClientClient: params.ControlPlaneClient,
169170
conn: params.ControlPlaneConn,
170171
}
171172
} else {
172-
conn, client, err := newClient(profile, c, c.additionalUnaryInterceptors, c.additionalStreamInterceptors)
173+
conn, client, err := newClient(ctx, profile, c, c.additionalUnaryInterceptors, c.additionalStreamInterceptors)
173174
if err != nil {
174175
return nil, fmt.Errorf("failed to create control plane client: %w", err)
175176
}
@@ -181,7 +182,7 @@ func NewClientWithOptions(params *ClientParams) (*Client, error) {
181182
return nil, fmt.Errorf("failed to start auth token manager: %w", err)
182183
}
183184

184-
logger.Debug("Modal client initialized successfully")
185+
logger.DebugContext(ctx, "Modal client initialized successfully")
185186

186187
c.Apps = &appServiceImpl{client: c}
187188
c.CloudBucketMounts = &cloudBucketMountServiceImpl{client: c}
@@ -200,7 +201,7 @@ func NewClientWithOptions(params *ClientParams) (*Client, error) {
200201

201202
// ipClient returns the input plane client for the given server URL.
202203
// It creates a new client if one doesn't exist for that specific server URL, otherwise it returns the existing client.
203-
func (c *Client) ipClient(serverURL string) (pb.ModalClientClient, error) {
204+
func (c *Client) ipClient(ctx context.Context, serverURL string) (pb.ModalClientClient, error) {
204205
c.mu.RLock()
205206
if client, ok := c.ipClients[serverURL]; ok {
206207
c.mu.RUnlock()
@@ -215,10 +216,10 @@ func (c *Client) ipClient(serverURL string) (pb.ModalClientClient, error) {
215216
return client, nil
216217
}
217218

218-
c.logger.Debug("Creating input plane client", "server_url", serverURL)
219+
c.logger.DebugContext(ctx, "Creating input plane client", "server_url", serverURL)
219220
prof := c.profile
220221
prof.ServerURL = serverURL
221-
conn, client, err := newClient(prof, c, c.additionalUnaryInterceptors, c.additionalStreamInterceptors)
222+
conn, client, err := newClient(ctx, prof, c, c.additionalUnaryInterceptors, c.additionalStreamInterceptors)
222223
if err != nil {
223224
return nil, err
224225
}
@@ -228,25 +229,26 @@ func (c *Client) ipClient(serverURL string) (pb.ModalClientClient, error) {
228229

229230
// Close stops the background auth token refresh and closes all gRPC connections.
230231
func (c *Client) Close() {
231-
c.logger.Debug("Closing Modal client")
232+
ctx := context.Background()
233+
c.logger.DebugContext(ctx, "Closing Modal client")
232234
c.authTokenManager.Stop()
233235

234236
if c.cpClient != nil {
235237
if err := c.cpClient.Close(); err != nil {
236-
c.logger.Warn("Failed to close control plane connection", "error", err)
238+
c.logger.WarnContext(ctx, "Failed to close control plane connection", "error", err)
237239
}
238240
}
239241

240242
c.mu.Lock()
241243
for serverURL, client := range c.ipClients {
242244
if err := client.Close(); err != nil {
243-
c.logger.Warn("Failed to close input plane connection", "server_url", serverURL, "error", err)
245+
c.logger.WarnContext(ctx, "Failed to close input plane connection", "server_url", serverURL, "error", err)
244246
}
245247
}
246248
c.ipClients = map[string]*clientWithConn{}
247249
c.mu.Unlock()
248250

249-
c.logger.Debug("Modal client closed")
251+
c.logger.DebugContext(ctx, "Modal client closed")
250252
}
251253

252254
// Version returns the SDK version.
@@ -298,7 +300,7 @@ func isRetryableGrpc(err error) bool {
298300

299301
// newClient dials the given server URL with auth/timeout/retry interceptors installed.
300302
// It returns (conn, stub). Close the conn when done.
301-
func newClient(profile Profile, c *Client, customUnaryInterceptors []grpc.UnaryClientInterceptor, customStreamInterceptors []grpc.StreamClientInterceptor) (*grpc.ClientConn, pb.ModalClientClient, error) {
303+
func newClient(ctx context.Context, profile Profile, c *Client, customUnaryInterceptors []grpc.UnaryClientInterceptor, customStreamInterceptors []grpc.StreamClientInterceptor) (*grpc.ClientConn, pb.ModalClientClient, error) {
302304
var target string
303305
var creds credentials.TransportCredentials
304306
var scheme string
@@ -314,7 +316,7 @@ func newClient(profile Profile, c *Client, customUnaryInterceptors []grpc.UnaryC
314316
return nil, nil, status.Errorf(codes.InvalidArgument, "invalid server URL: %s", profile.ServerURL)
315317
}
316318

317-
c.logger.Debug("Connecting to Modal server", "target", target, "scheme", scheme)
319+
c.logger.DebugContext(ctx, "Connecting to Modal server", "target", target, "scheme", scheme)
318320

319321
unaryInterceptors := []grpc.UnaryClientInterceptor{
320322
headerInjectorUnaryInterceptor(profile, c.sdkVersion),

modal-go/cloud_bucket_mount.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package modal
22

33
import (
4+
"context"
45
"fmt"
56
"net/url"
67
"strings"
@@ -39,6 +40,7 @@ type CloudBucketMountParams struct {
3940

4041
// New creates a new CloudBucketMount.
4142
func (s *cloudBucketMountServiceImpl) New(bucketName string, params *CloudBucketMountParams) (*CloudBucketMount, error) {
43+
ctx := context.Background()
4244
if params == nil {
4345
params = &CloudBucketMountParams{}
4446
}
@@ -67,7 +69,8 @@ func (s *cloudBucketMountServiceImpl) New(bucketName string, params *CloudBucket
6769
} else {
6870
mount.bucketType = pb.CloudBucketMount_S3
6971
if s.client != nil && s.client.logger != nil {
70-
s.client.logger.Debug(
72+
s.client.logger.DebugContext(
73+
ctx,
7174
"CloudBucketMount received unrecognized bucket endpoint URL. Assuming AWS S3 configuration as fallback.",
7275
"BucketEndpointURL", *mount.BucketEndpointURL,
7376
)

modal-go/function.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ func (f *Function) createRemoteInvocation(ctx context.Context, input *pb.Functio
305305
}
306306
inputPlaneURL := metadata.GetInputPlaneUrl()
307307
if inputPlaneURL != "" {
308-
ipClient, err := f.client.ipClient(inputPlaneURL)
308+
ipClient, err := f.client.ipClient(ctx, inputPlaneURL)
309309
if err != nil {
310310
return nil, err
311311
}

0 commit comments

Comments
 (0)