aws / aws-sdk-go

AWS SDK for the Go programming language.
http://aws.amazon.com/sdk-for-go/
Apache License 2.0
8.63k stars 2.07k forks source link

Add context.Context to aws.Logger API #3398

Closed rittneje closed 3 years ago

rittneje commented 4 years ago

We have a bunch of information passed around in context.Context (such as request id) that we want included in all logs coming from that context's scope. However, at present that context does not get passed to the aws.Logger instance, since there's no API parameter for it. This means that the debug logs that are emitted do not include this additional information.

I proposed deprecated the existing aws.Logger interface and replacing it with:

package aws 

...

type ContextLogger interface {
    LogWithContext(ctx context.Context, v ...interface{})
}

With regards to aws.Config, there are two choices:

  1. The Logger field remains as-is. For backwards compatibility, if the provided aws.Logger does not also implement aws.ContextLogger, then Log will be called (without the context as a parameter).
  2. The Logger field is deprecated and replaced with a new ContextLogger field of type aws.ContextLogger. At runtime, use ContextLogger if set, otherwise use Logger if set.

ETA: From looking at the SDK, it would also be handy if the log method were replaced with level-specific methods.

package aws 

...

type ContextLogger interface {
    LogDebug(ctx context.Context, v ...interface{})
    LogInfo(ctx context.Context, v ...interface{})
    LogWarning(ctx context.Context, v ...interface{})
    LogError(ctx context.Context, v ...interface{})
}
diehlaws commented 4 years ago

Hi @rittneje, thanks for reaching out to us about this. You can define a custom logger to use with a service client and pass the context for a call via the logger's Log() method to print the context details in the same location as the rest of the debug logs:

myLogger := aws.LoggerFunc(func(ctx context.Context, args ...interface{}) {
  fmt.Fprintln(os.Stdout, args...)
})
sess := session.New(&aws.Config{
  Region:   aws.String("us-west-2"),
  LogLevel: aws.LogLevel(aws.LogDebugWithSigning),
  Logger:   myLogger,
})
svc := s3.New(sess)

ctx := context.WithValue(context.Background(), "foo", "bar")
myLogger.Log("Context:", ctx)

If this does not work for your use case please provide more details about your use case so we can better understand the need for the feature you're requesting.

rittneje commented 4 years ago

@diehlaws No, that will not work. The context is a per-request concept.

For example, suppose I am using the dynamodb package.

type logger struct{}

func (logger) LogWithContext(ctx context.Context, v ...interface{}) {
    // include request id in the log, for ease of traceability/searching
    fmt.Println(ctx.Value("request-id"), fmt.Sprint(v...))
}

config := &aws.Config{
    Logger: logger{},
    ...,
}
dbClient := dynamodb.New(config)

...

func handleRequest(rw http.ResponseWriter, req *http.Request) {
    ...

    ctx := context.WithValue(req.Context(), "request-id", req.Header.Get("X-Request-ID"))

    dbClient.GetItemWithContext(ctx, ...)

    ...
}

The context passed to GetItemWithContext needs to be passed all the way through to the logger (for those logs made by the call to GetItemWithContext). However, since the Logger.Log method does not take a context.Context parameter, there is no way to do this without making a separate *dynamodb.DynamoDB instance for each request, which does not really make sense.

rittneje commented 4 years ago

@diehlaws Just following up on this.

rittneje commented 4 years ago

@diehlaws I raised #3485 with the necessary changes. This patch is invaluable for troubleshooting, as the existing aws.Logger interface is unusable in any multi-threaded environment.

fsenart commented 4 years ago

To put my two cents in, it goes further than a multi-threaded environment. It's about how to add contextual information to each log message. The introduction of context in the SDK has been a giant leap forward, but it has been used mostly for cancellation purposes. Though, there is more to contexts. In a distributed environment, we need contextual information (request id, correlation ids, trace ids, etc.) to make sense of running services. The natural place of this kind of transient information is in the context and the lack of the context in the logger interface prevents the enrichment of log messages emitted by the SDK.

skmcgrail commented 3 years ago

The V2 Go SDK has support for loggers that are context-aware, see Context Aware Loggers. We encourage you to look at migrating your applications to the V2 SDK to take advantage of this feature.

github-actions[bot] commented 3 years ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.