Introduce `GetBackendWithConfig` and make logging configurable per backend #611

brandur · 2018-06-29T01:47:07Z

Introduces a new system for getting backends with a set of custom
configuration that's a little cleaner than adding a new setter for every
option that we add in the future.

BackendConfig is introduced a user configurable struct which is then
passed to GetBackendWithConfig, which returns a backend configured to
spec. GetBackend remains, is backward compatible, and continues to set
a default backend for either type.

A new option on BackendConfig is being able to set a per-backend
Logger and LogLevel. This is the impetus that sparked this change as
documented in #608.

So the unfortunate part is that BackendConfig is confusingly named
similarly to the very poorly named BackendConfiguration, which is the
final implementation of a backend. I wanted to keep this patch backward
compatible, but I want to make a breaking change in the future to rename
this to backendImplementation and make it unexported at the same time
because it shouldn't be needed outside of the core package.

We should also unexport NewBackends at the same time. It was
introduced to allow some configuration for backends, but it was a pretty
bad idea.

Fixes #608.

r? @remi-stripe

brandur-stripe · 2018-06-29T01:48:55Z

stripe_test.go

@@ -15,7 +15,7 @@ import (
 )

 func TestBearerAuth(t *testing.T) {
-	c := &stripe.BackendConfiguration{URL: stripe.APIURL}
+	c := stripe.GetBackend(stripe.APIBackend).(*stripe.BackendConfiguration)


Dang it. I kind of punted on finding the right way to solve this for now.

It's an improvement that we now use GetBackend to get a default backend with good values set, but it's still bad that we then need to cast it back to BackendConfiguration. The problem is that each of these tests uses BackendConfiguration.NewRequest, and that is a function that's not part of the Backend interface.

We should either add it to the interface, or find a better way to write these tests, with a preference for the latter.

brandur-stripe · 2018-06-29T01:50:58Z

order/client_test.go

 	})
+	orderClient := Client{B: backend, Key: stripe.Key}


Instantiating a new client turned out to be a far cleaner way to implement this test anyway (the defer to reset the default backend at the top is no longer needed).

SamWhited · 2018-06-29T02:41:59Z

stripe.go

-// BackendConfiguration is the internal implementation for making HTTP calls to Stripe.
+// BackendConfig is used to configure a new Stripe backend.
+type BackendConfig struct {
+	// HTTPClient is an HTTP client instance to use when maxing requests.


s/maxing/making/

Thanks! Fixed.

SamWhited · 2018-06-29T02:49:48Z

stripe.go

+	//
+	// Defaults to 0 (no logging), so please make sure to set this if you want
+	// to see logging output in your custom configuration.
+	LogLevel int


I don't think this is ever used; should it have a value in BackendConfiguration and be used there too? Given that it will default to zero it may not be possible to determine if we should use this or the global LogLevel, I'm not sure how to do that in a backwards compatible way.

I don't think this is ever used; should it have a value in BackendConfiguration and be used there too?

Haha, that's what I get for trying to push through a change like this near the end of the day!

Yep, that's a major omission. I just added LogLevel to BackendConfiguration and changed all existing checks on LogLevel to use it.

SamWhited · 2018-06-29T02:50:13Z

stripe.go

+	// problem.
+	//
+	// Defaults to 0.
+	MaxNetworkRetries int


Same as LogLevel.

Thanks! Fixed that as well. (Its value was already on BackendConfiguration, but it was being carried over from the config struct.)

SamWhited · 2018-06-29T02:50:50Z

Wow, that was fast; thanks for implementing this!

brandur-stripe · 2018-06-29T03:39:59Z

Given that it will default to zero it may not be possible to determine if we should use this or the global LogLevel, I'm not sure how to do that in a backwards compatible way.

Yeah, the inability to check on 0 versus unset is frustrating.

I believe I've implemented this to be backwards compatible though — when getting a default backend (i.e., not through GetBackendWithConfig), we simply always set backendConfig.LogLevel to stripe.LogLevel, so it'll carry through.

There are still problems in other areas though. For example, if we want to give either LogLevel or MaxNetworkRetries a default value that's not 0 when passing a BackendConfig (which is quite plausible), we then won't be able to tell whether a user has left them unset or explicitly set them to 0.

One possibility is to set these to *int64 like we do in our *Params structs and ask people to use the stripe.Int64(...) helper to set them. It's still a little weird given that HTTPClient, Logger etc. aren't set that way, but maybe it's fine. Thoughts?

brandur-stripe · 2018-06-29T03:40:11Z

(Oh, and thanks for the review @SamWhited!)

brandur-stripe · 2018-06-29T03:41:28Z

stripe.go

@@ -233,8 +278,8 @@ func (s *BackendConfiguration) NewRequest(method, path, key, contentType string,
 // the backend's HTTP client to execute the request and unmarshals the response
 // into v. It also handles unmarshaling errors returned by the API.
 func (s *BackendConfiguration) Do(req *http.Request, v interface{}) error {
-	if LogLevel > 1 {
-		Logger.Printf("Requesting %v %v%v\n", req.Method, req.URL.Host, req.URL.Path)
+	if s.LogLevel > 1 {


We should also make helpers for logInfo, logError, logDebug so we don't have these ugly checks on LogLevel everywhere, but I'll tackle that in another PR.

SamWhited · 2018-06-29T03:58:01Z

We should also make helpers for logInfo, logError, logDebug so we don't have these ugly checks on LogLevel everywhere, but I'll tackle that in another PR.
…
One possibility is to set these to *int64 like we do in our *Params structs and ask people to use the stripe.Int64(...) helper to set them. It's still a little weird given that HTTPClient, Logger etc. aren't set that way, but maybe it's fine. Thoughts?

Depending on how closely you want this to match what's already being done globally, one option might be to get rid of LogLevel in the config struct and have separate loggers for each level, eg. LogInfo *log.Logger, LogError *log.Logger, etc. and set each one to a nop Printerf implementation by default.

brandur-stripe · 2018-06-29T23:34:03Z

Depending on how closely you want this to match what's already being done globally, one option might be to get rid of LogLevel in the config struct and have separate loggers for each level, eg. LogInfo *log.Logger, LogError *log.Logger, etc. and set each one to a nop Printerf implementation by default.

Ah, true. I'd be a little hesitant to introduce a new paradigm here though — leveled logging where the wanted level is set just once is a pretty widespread paradigm (even in Go).

I just looked at Zap and Logrus, two popular libraries, and they both basically use an identical convention here:

log.SetLevel(log.WarnLevel)

(Which is probably roughly what we should be doing as well, even if it looks like just setting LogLevel on BackendConfig.)

The one major departure compared to what we have is that normally the level is set on the logging object itself instead of as a separate field on the construct that holds the logger. I kind of want to change Printfer to include more functions to do something similar:

type Printfer interface {
	Errorf(format string, v ...interface{})
	Infof(format string, v ...interface{})
        SetLogLevel(...)
	Warnf(format string, v ...interface{})
}

This would make using Zap and Logrus easy, but then of course Go's built in logging module would no longer comply to the interface. Sigh.

I think the right thing to do for now is to leave it more or less as I have here, and we can decide on a larger breaking change later if it comes up again.

remi-stripe

some minor comments but otherwise this looks good to me.
To be honest I don't 100% follow the logic/usage of the code though but I think I understood the changes

remi-stripe · 2018-07-03T17:59:34Z

stripe.go

+	backend = GetBackendWithConfig(
+		backendType,
+		&BackendConfig{
+			HTTPClient:        httpClient,


why pass all this since the method seems to set the default for us?

Good eye. Yeah, I went back in forth on this — I settled for what you see because I felt that the explicitness is somewhat advantageous compared to setting a bunch of 0/nil and then having to follow the code down to GetBackendWithConfig to see what's going to end up in there.

I don't feel too strongly about it though, and am happy to go with the other way too. What do you think?

I don't feel strongly either so I'm fine leaving as is

remi-stripe · 2018-07-03T18:14:43Z

stripe.go

+		// to fulfill the principle of least astonishment.
+		config.URL += "/v1"
+
+		return &BackendConfiguration{


this like the code is identical between both switch except for the url, could we generalize that?

Great point. I just updated the code and it looks way cleaner now.

brandur-stripe · 2018-07-03T18:52:29Z

To be honest I don't 100% follow the logic/usage of the code though but I think I understood the changes

Yeah sorry, but unfortunately the relations of Backend, BackendConfig, BackendConfiguration, SupportedBackend, etc. are all still pretty confusing right now :/

I think I can clean this up a lot in the future and make this way easier to wrap your head around, but I wanted to target a non-breaking change for this PR and see if we can improve things a bit later.

ptal @remi-stripe Updated the patch, please take one more look.

remi-stripe

LGTM

…ckend Introduces a new system for getting backends with a set of custom configuration that's a little cleaner than adding a new setter for every option that we add in the future. `BackendConfig` is introduced a user configurable struct which is then passed to `GetBackendWithConfig`, which returns a backend configured to spec. `GetBackend` remains, is backward compatible, and continues to set a default backend for either type. A new option on `BackendConfig` is being able to set a per-backend `Logger` and `LogLevel`. This is the impetus that sparked this change as documented in #608. So the unfortunate part is that `BackendConfig` is confusingly named similarly to the very poorly named `BackendConfiguration`, which is the final implementation of a backend. I wanted to keep this patch backward compatible, but I want to make a breaking change in the future to rename this to `backendImplementation` and make it unexported at the same time because it shouldn't be needed outside of the core package. We should also unexport `NewBackends` at the same time. It was introduced to allow some configuration for backends, but it was a pretty bad idea. Fixes #608.

brandur-stripe · 2018-07-03T19:08:59Z

Thanks Remi!

brandur-stripe · 2018-07-03T19:10:02Z

Released as 35.9.0.

SamWhited · 2018-07-03T19:38:10Z

Released as 35.9.0.

Fantastic, thanks again for your quick response! This will let me clean up a lot of hacky race-condition-prone stuff on my end where I had to make some logging global to pass it between modules and apply it to Stripe. Looking forward to cleaning that up!

brandur-stripe · 2018-07-03T21:02:29Z

Fantastic, thanks again for your quick response! This will let me clean up a lot of hacky race-condition-prone stuff on my end where I had to make some logging global to pass it between modules and apply it to Stripe. Looking forward to cleaning that up!

No worries! I hope it works out.

In #611 I accidentally introduced a regression in the `NewBackends` function in that an `http.Client` passed into the function isn't actually wired through to the new backends. Here we fix that problem by changing the invocations to use `GetBackendWithConfig` like they were supposed to. Fixes #626.

* Adding workplan * Extracting out price comparison helper * PUlling transform_salesforce_billing_type_to_usage_type to functional helpers * Extract out tiered params sanitization * Eliminate sf_cpq_term_interval helper * Extract out custom price detection logic * Test for price helpers * Fixing mapper tests Tests removed are already covered in another test * Bump sfdx version to maybe fix ci issue * Fix typing error

stripe-ci assigned remi-stripe Jun 29, 2018

brandur-stripe reviewed Jun 29, 2018

View reviewed changes

brandur-stripe force-pushed the brandur-backend-config branch from c81a193 to 24e7655 Compare June 29, 2018 01:49

brandur-stripe reviewed Jun 29, 2018

View reviewed changes

brandur-stripe mentioned this pull request Jun 29, 2018

Feature request: non-global logging #608

Closed

SamWhited reviewed Jun 29, 2018

View reviewed changes

brandur-stripe force-pushed the brandur-backend-config branch 2 times, most recently from 4f9ffc7 to bfb2d9b Compare June 29, 2018 03:33

brandur-stripe reviewed Jun 29, 2018

View reviewed changes

remi-stripe reviewed Jul 3, 2018

View reviewed changes

brandur-stripe force-pushed the brandur-backend-config branch from bfb2d9b to 80b6840 Compare July 3, 2018 18:50

remi-stripe approved these changes Jul 3, 2018

View reviewed changes

stripe-ci added the approved label Jul 3, 2018

brandur-stripe force-pushed the brandur-backend-config branch from 80b6840 to 647da42 Compare July 3, 2018 19:04

stripe-ci removed the approved label Jul 3, 2018

brandur-stripe merged commit 7cc772a into master Jul 3, 2018

brandur-stripe deleted the brandur-backend-config branch July 3, 2018 19:09

brandur mentioned this pull request Jul 17, 2018

Wire an http.Client from NewBackends through to backends #627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `GetBackendWithConfig` and make logging configurable per backend #611

Introduce `GetBackendWithConfig` and make logging configurable per backend #611

brandur commented Jun 29, 2018

brandur-stripe Jun 29, 2018

brandur-stripe Jun 29, 2018

SamWhited Jun 29, 2018

brandur-stripe Jun 29, 2018

SamWhited Jun 29, 2018

brandur-stripe Jun 29, 2018

SamWhited Jun 29, 2018

brandur-stripe Jun 29, 2018

SamWhited commented Jun 29, 2018

brandur-stripe commented Jun 29, 2018

brandur-stripe commented Jun 29, 2018

brandur-stripe Jun 29, 2018

SamWhited commented Jun 29, 2018 •

edited

Loading

brandur-stripe commented Jun 29, 2018

remi-stripe left a comment

remi-stripe Jul 3, 2018

brandur-stripe Jul 3, 2018

remi-stripe Jul 3, 2018

remi-stripe Jul 3, 2018

brandur-stripe Jul 3, 2018

brandur-stripe commented Jul 3, 2018

remi-stripe left a comment

brandur-stripe commented Jul 3, 2018

brandur-stripe commented Jul 3, 2018

SamWhited commented Jul 3, 2018

brandur-stripe commented Jul 3, 2018

Introduce GetBackendWithConfig and make logging configurable per backend #611

Introduce GetBackendWithConfig and make logging configurable per backend #611

Conversation

brandur commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamWhited commented Jun 29, 2018

brandur-stripe commented Jun 29, 2018

brandur-stripe commented Jun 29, 2018

Choose a reason for hiding this comment

SamWhited commented Jun 29, 2018 • edited Loading

brandur-stripe commented Jun 29, 2018

remi-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented Jul 3, 2018

remi-stripe left a comment

Choose a reason for hiding this comment

brandur-stripe commented Jul 3, 2018

brandur-stripe commented Jul 3, 2018

SamWhited commented Jul 3, 2018

brandur-stripe commented Jul 3, 2018

Introduce `GetBackendWithConfig` and make logging configurable per backend #611

Introduce `GetBackendWithConfig` and make logging configurable per backend #611

SamWhited commented Jun 29, 2018 •

edited

Loading