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

Add Stripe client telemetry to request headers #766

Merged
merged 18 commits into from
Jan 17, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions stripe.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ var LogLevel = 2
// be overridden if a backend is created with GetBackendWithConfig.
var Logger Printfer

// EnableTelemetry allows request metrics (request id and duration) to be sent
// to Stripe in subsequent requests via the `X-Stripe-Client-Telemetry` header.
var EnableTelemetry = false

//
// Public types
//
Expand Down Expand Up @@ -140,6 +144,13 @@ type BackendConfig struct {
URL string
}

// RequestMetrics contains the payload sent in the `X-Stripe-Client-Telemetry`
// header when stripe.EnableTelemetry = true.
type RequestMetrics struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this requestMetrics instead so that it's not exported outside the package (whether the first letter is lower case or upper case determines this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially did make RequestMetrics private, however I wanted the tests to ensure that I could unmarshal the sent metrics back into this struct. Do you think it's ok to omit that check? Otherwise, I could duplicate the requestMetrics definition in stripe_test.go for the sake of unmarshaling.

Copy link
Contributor

@brandur-stripe brandur-stripe Jan 14, 2019

Choose a reason for hiding this comment

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

Oops, sorry I missed this.

I actually forgot that stripe_test.go is in its own stripe_test package. IMO, we should probably just put it in stripe. All the tests in subpackages are just in the same namespace as their accompanying file (customer/, charge/, form/, etc.) and even most of the *_test.go in the top-level package are as well.

RequestID string `json:"request_id"`
RequestDurationMS int `json:"request_duration_ms"`
}

// BackendImplementation is the internal implementation for making HTTP calls
// to Stripe.
//
Expand All @@ -158,6 +169,7 @@ type BackendImplementation struct {
//
// See also SetNetworkRetriesSleep.
networkRetriesSleep bool
lastRequestMetrics *RequestMetrics
Copy link
Contributor

Choose a reason for hiding this comment

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

Because networkRetriesSleep above has its own comment, could you just add a newline above this one just to show that it's not part of the same group?

}

// Call is the Backend.Call implementation for invoking Stripe APIs.
Expand Down Expand Up @@ -292,8 +304,15 @@ func (s *BackendImplementation) Do(req *http.Request, body *bytes.Buffer, v inte
s.Logger.Printf("Requesting %v %v%v\n", req.Method, req.URL.Host, req.URL.Path)
}

if EnableTelemetry && s.lastRequestMetrics != nil {
metricsJSON, _ := json.Marshal(s.lastRequestMetrics)
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is unlikely to happen, but let's not swallow errors just in case we introduce something in the future that becomes difficult to debug because we're throwing some message away. Observe err and handle if it's non-nil like elsewhere.

payload := fmt.Sprintf(`{"last_request_metrics":%s}`, metricsJSON)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for cleanliness, could you create a wrapper struct for this one instead of manually assembling the JSON string (maybe requestTelemetry)?

req.Header.Set("X-Stripe-Client-Telemetry", payload)
}

var res *http.Response
var err error
var requestDurationMS int
for retry := 0; ; {
start := time.Now()

Expand Down Expand Up @@ -338,8 +357,14 @@ func (s *BackendImplementation) Do(req *http.Request, body *bytes.Buffer, v inte
}
}

// `requestStart` is used solely for client telemetry and, unlike `start`,
// does not account for the time spent building the request body.
requestStart := time.Now()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use start? All that's happening between the two is some structs being initialized locally. In practice it's likely to have a negligible effect on timing, and I don't think it's worth complicating the code over.


res, err = s.HTTPClient.Do(req)

requestDurationMS = int(time.Since(requestStart) / time.Millisecond)

if s.LogLevel > 2 {
s.Logger.Printf("Request completed in %v (retry: %v)\n",
time.Since(start), retry)
Expand Down Expand Up @@ -387,6 +412,16 @@ func (s *BackendImplementation) Do(req *http.Request, body *bytes.Buffer, v inte
return err
}

if EnableTelemetry {
reqID := res.Header.Get("Request-Id")
if len(reqID) > 0 {
s.lastRequestMetrics = &RequestMetrics{
RequestID: reqID,
RequestDurationMS: requestDurationMS,
}
}
}

defer res.Body.Close()

resBody, err := ioutil.ReadAll(res.Body)
Expand Down Expand Up @@ -886,6 +921,7 @@ func newBackendImplementation(backendType SupportedBackend, config *BackendConfi
Type: backendType,
URL: config.URL,
networkRetriesSleep: true,
lastRequestMetrics: nil,
}
}

Expand Down
136 changes: 136 additions & 0 deletions stripe_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"encoding/json"
"fmt"
"net/http"
"net/http/httptest"
"regexp"
Expand Down Expand Up @@ -165,6 +166,141 @@ func TestDo_RetryOnTimeout(t *testing.T) {
assert.Equal(t, uint32(2), atomic.LoadUint32(&counter))
}

// Test that telemetry metrics are not sent by default
func TestDo_TelemetryDisabled(t *testing.T) {
type testServerResponse struct {
Message string `json:"message"`
}

message := "Hello, client."
requestNum := 0

testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Copy link

Choose a reason for hiding this comment

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

Great test. 👍 I love Go's httptest package!

// none of the requests should include telemetry metrics
assert.Equal(t, r.Header.Get("X-Stripe-Client-Telemetry"), "")

response := testServerResponse{Message: message}

data, err := json.Marshal(response)
assert.NoError(t, err)

_, err = w.Write(data)
assert.NoError(t, err)

requestNum++
}))
defer testServer.Close()

backend := stripe.GetBackendWithConfig(
stripe.APIBackend,
&stripe.BackendConfig{
LogLevel: 3,
MaxNetworkRetries: 0,
URL: testServer.URL,
},
).(*stripe.BackendImplementation)

for i := 0; i < 2; i++ {
Copy link

Choose a reason for hiding this comment

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

It's initially unclear why this loop executes twice. Can you add a quick comment that explains that it's because the telemetry comes from the previous completed request/response?

request, err := backend.NewRequest(
http.MethodGet,
"/hello",
"sk_test_123",
"application/x-www-form-urlencoded",
nil,
)
assert.NoError(t, err)

var response testServerResponse
err = backend.Do(request, nil, &response)

assert.NoError(t, err)
assert.Equal(t, message, response.Message)
}

// We should have seen exactly two requests.
assert.Equal(t, 2, requestNum)
}

// Test that telemetry metrics are sent on subsequent requests when
// stripe.EnableTelemetry = true.
func TestDo_TelemetryEnabled(t *testing.T) {
stripe.EnableTelemetry = true
defer func() {
stripe.EnableTelemetry = false
}()

type testServerResponse struct {
Message string `json:"message"`
}

type clientTelemetry struct {
LastRequestMetrics stripe.RequestMetrics `json:"last_request_metrics"`
}

message := "Hello, client."
requestNum := 0

testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
telemetryStr := r.Header.Get("X-Stripe-Client-Telemetry")
switch requestNum {
Copy link

Choose a reason for hiding this comment

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

👍

case 0:
// the first request should not receive any metrics
assert.Equal(t, telemetryStr, "")
case 1:
// the second request should include the metrics for the first request
assert.Contains(t, telemetryStr, `"request_id":"req_0"`)

// the telemetry should properly unmarshal into stripe.RequestMetrics
var telemetry clientTelemetry
err := json.Unmarshal([]byte(telemetryStr), &telemetry)
assert.NoError(t, err)
default:
assert.Fail(t, "Should not have reached request %v", requestNum)
}

w.Header().Set("Request-Id", fmt.Sprintf("req_%d", requestNum))
response := testServerResponse{Message: message}

data, err := json.Marshal(response)
assert.NoError(t, err)

_, err = w.Write(data)
assert.NoError(t, err)

requestNum++
}))
defer testServer.Close()

backend := stripe.GetBackendWithConfig(
stripe.APIBackend,
&stripe.BackendConfig{
LogLevel: 3,
MaxNetworkRetries: 0,
URL: testServer.URL,
},
).(*stripe.BackendImplementation)

for i := 0; i < 2; i++ {
request, err := backend.NewRequest(
http.MethodGet,
"/hello",
"sk_test_123",
"application/x-www-form-urlencoded",
nil,
)
assert.NoError(t, err)

var response testServerResponse
err = backend.Do(request, nil, &response)

assert.NoError(t, err)
assert.Equal(t, message, response.Message)
}

// We should have seen exactly two requests.
assert.Equal(t, 2, requestNum)
}

func TestFormatURLPath(t *testing.T) {
assert.Equal(t, "/v1/resources/1/subresources/2",
stripe.FormatURLPath("/v1/resources/%s/subresources/%s", "1", "2"))
Expand Down