Skip to content
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.

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

Distributed tracing and other context in logging #1281

Open
mickeyreiss opened this issue May 12, 2021 · 2 comments
Open

Distributed tracing and other context in logging #1281

mickeyreiss opened this issue May 12, 2021 · 2 comments
Labels

Comments

@mickeyreiss
Copy link

The current logging interface does not seem to allow callers to inject structured fields, such as trace ids, into the log lines. While custom loggers are acceptable, only global instances are acceptable, and the logging interface only passes Stripe's log messages as arguments.

In my setup, I use context.Context to pass request ids into my logger, which in turn includes these request ids in the structured log lines that are written to stderr. Systems like OpenCensus/OpenTelemetry/OpenTracing also use context to pass correlation tokens through.

It would be nice if the Stripe SDK had support for this, so that I could more easily correlate Stripe's logging with incoming requests. While it is possible to do this to a certain extent by overriding the http.Transaport, I'd like to take full advantage of the diagnostic logging that is already built into this library.

I am curious if this has already been thought through, or if others have solutions. If not, it would be great to add support for this into the logging interfaces.


The current API, for reference:

stripe-go/stripe.go

Lines 181 to 195 in ebbff36

// LeveledLogger is the logger that the backend will use to log errors,
// warnings, and informational messages.
//
// LeveledLoggerInterface is implemented by LeveledLogger, and one can be
// initialized at the desired level of logging. LeveledLoggerInterface
// also provides out-of-the-box compatibility with a Logrus Logger, but may
// require a thin shim for use with other logging libraries that use less
// standard conventions like Zap.
//
// Defaults to DefaultLeveledLogger.
//
// To set a logger that logs nothing, set this to a stripe.LeveledLogger
// with a Level of LevelNull (simply setting this field to nil will not
// work).
LeveledLogger LeveledLoggerInterface

stripe-go/log.go

Lines 124 to 142 in ebbff36

// LeveledLoggerInterface provides a basic leveled logging interface for
// printing debug, informational, warning, and error messages.
//
// It's implemented by LeveledLogger and also provides out-of-the-box
// compatibility with a Logrus Logger, but may require a thin shim for use with
// other logging libraries that you use less standard conventions like Zap.
type LeveledLoggerInterface interface {
// Debugf logs a debug message using Printf conventions.
Debugf(format string, v ...interface{})
// Errorf logs a warning message using Printf conventions.
Errorf(format string, v ...interface{})
// Infof logs an informational message using Printf conventions.
Infof(format string, v ...interface{})
// Warnf logs a warning message using Printf conventions.
Warnf(format string, v ...interface{})
}

@remi-stripe
Copy link
Contributor

@mickeyreiss Thanks a lot for the feedback/idea. I don't think it's something I've seen asked before or that we've considered. We'll have a look into potential improvements and circle back!

@yareid
Copy link

yareid commented Sep 23, 2022

This is something we'd be very interested in too. We do provide our own LeveledLogger which integrates the Stripe logging into our log stream, but we are unable to surface our tenant or request identifiers without access to the context.Context of the call.

We deal with a large number of Stripe accounts for different tenants (a mix of Stripe Connect and stand-alone accounts). While the Stripe log message do contain a Stripe request_id it can be time consuming to reverse this back to a particular Stripe Account or tenant on our side.

I can think of a couple of possible approaches.

Allow a call specific logger to be provided via stripe.Params
We provide a context.Context for each call anyhow (which does give us http span monitoring). It would be easy for us to provide a call specific LeveledLogger also.

	params := &stripe.PaymentIntentParams{}
	params.Context = ctx
	params.Logger = logger
	params.SetStripeAccount(acct)

	pi, err := api.PaymentIntents.Get(piID, params)

Add a ContextLeveledLogger interface
And prefer to use this if it is provided when the SDK is instantiated. You would need to pass the context from the call stripe.Params to it.

 type ContextLeveledLoggerInterface interface { 
 	// Debugf logs a debug message using Printf conventions. 
 	Debugf(ctx context.Context, format string, v ...interface{}) 

 	// etc

Our implementation of this interface could then extract a tenant and request identifier from the context -- or more likely an entire logger.

Thanks for considering these options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants