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 #518

Merged
merged 1 commit into from
Jan 15, 2019

Conversation

jameshageman-stripe
Copy link
Contributor

Follows stripe/stripe-ruby#696 and stripe/stripe-php#549 in adding telemetry metadata to request headers.

The telemetry is disabled by default, and can be enabled like so:

stripe.enable_telemetry = True

@remi-stripe
Copy link
Contributor

cc @stripe/api-libraries for awareness

@jameshageman-stripe jameshageman-stripe changed the title [WIP] Add Stripe client telemetry to request headers Add Stripe client telemetry to request headers Jan 11, 2019
@jameshageman-stripe
Copy link
Contributor Author

Pinging @akropp-stripe @ob-stripe @brandur-stripe for review as you were all part of stripe/stripe-ruby#696.

The 2 failing tests are also failing on master and are unrelated to my changes.

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

Minor comment below, but generally LGTM! Thanks for following the same conventions set by the Ruby version.

OB's informally the main Python maintainer, so I'll leave final review to him.

@@ -12,6 +12,7 @@
from stripe.multipart_data_generator import MultipartDataGenerator
from stripe.six.moves.urllib.parse import urlencode, urlsplit, urlunsplit
from stripe.stripe_response import StripeResponse
from stripe.request_metrics import RequestMetrics
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to arrange this as alphabetically as possible (since it's already roughly in that order)? Just swap it with the line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -0,0 +1,10 @@
class RequestMetrics(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note, but the one in Ruby is called StripeRequestMetrics, and it'd be nice to standardize on a name. RequestMetrics is probably better given it's already in the Stripe namespace.

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'm fine either way, I just figured from stripe.stripe_request_metrics import StripeRequestMetrics was overly verbose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the Stripe prefix seems pretty redundant. Let's stick with what you have.

@brandur-stripe
Copy link
Contributor

@ob-stripe I looked into the test failures that are happening on master a bit. They're caused because our TestPycurlClientHttpProxy and TestPycurlClientHttpsProxy classes inherit from TestPycurlClient and are calling that parent's check_call implementation (see the last line below):

class TestPycurlClientHttpProxy(TestPycurlClient):
     ...

    @pytest.fixture
    def check_call(self, request_mocks, curl_mock):
        def check_call(mock, meth, url, post_data, headers):
            ...

            super(TestPycurlClientHttpProxy, self).check_call(request_mocks)(
                mock, meth, url, post_data, headers)

Because all our check_calls are marked as @pytest.fixture, Python doesn't expect any explicit invocations, and therefore this error is triggered (I assume that something must have changed in some library because this seemed to be fine before).

I could fix this by just moving TestPycurlClient's check_call body into a new method (check_call_inner?) and just having check_call as well as TestPycurlClientHttpProxy and TestPycurlClientHttpsProxy call that, but I'm not sure that's a "clean" solution. Can you think of a better way?

@ob-stripe
Copy link
Contributor

@jameshageman-stripe Can you rebase on the latest master? I fixed the unrelated test error.

@brandur-stripe Oops, sorry, I'm only seeing your comment now! FWIW I initially went with your suggestion, but changed my mind to make the new method slightly more consistent with what we already do in TestRequestsClient.check_call.

@brandur-stripe
Copy link
Contributor

@brandur-stripe Oops, sorry, I'm only seeing your comment now! FWIW I initially went with your suggestion, but changed my mind to make the new method slightly more consistent with what we already do in TestRequestsClient.check_call.

NP. Your fix ended up being better than what I was imagining.

@jameshageman-stripe
Copy link
Contributor Author

jameshageman-stripe commented Jan 14, 2019

@ob-stripe Rebased: all tests are passing except pypy3, which is failing with ImportError: cannot import name 'pythonapi'. This appears to be happening on master as well, so I'd say it is unrelated to this PR's changes.

@jameshageman-stripe
Copy link
Contributor Author

r? @ob-stripe

@@ -0,0 +1,10 @@
class RequestMetrics(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add from __future__ import absolute_import, division, print_function at the beginning of the file? Even though that line won't do anything here, it's present in all source files to prevent Python 2/3 compatibility issues.

@ob-stripe
Copy link
Contributor

Requested one minor change, but otherwise LGTM!

@ob-stripe Rebased: all tests are passing except pypy3, which is failing with ImportError: cannot import name 'pythonapi'. This appears to be happening on master as well, so I'd say it is unrelated to this PR's changes.

Yes, this is an issue with pipenv that will be fixed in the next version. Sorry, should have let you know that was expected!

@jameshageman-stripe
Copy link
Contributor Author

@ob-stripe Done!

@ob-stripe
Copy link
Contributor

@jameshageman-stripe Thanks! Mind squashing the commits before I merge and cut a release?

@jameshageman-stripe
Copy link
Contributor Author

@ob-stripe squashed

@ob-stripe ob-stripe self-assigned this Jan 15, 2019
@ob-stripe ob-stripe merged commit c7cf8f0 into stripe:master Jan 15, 2019
@ob-stripe
Copy link
Contributor

Released as 2.18.0.

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

Successfully merging this pull request may close these issues.

None yet

4 participants