-
Notifications
You must be signed in to change notification settings - Fork 12
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
Adding customer-type tag to header for API #58
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After my review let's push this to Cloud Run containers in GCP and make sure it still works.
Also, I like the dash case used for customer-type. I think in the future I will use that to represent the sharing of came'Case with snake_case.
@@ -71,18 +71,24 @@ def process_order(cart): | |||
|
|||
@app.before_request | |||
def sentry_event_context(): | |||
print('\nrequest.headers email', request.headers.get('email')) | |||
# print('\nrequest.headers email', request.headers.get('email')) | |||
global Inventory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this global variable declaration is the only thing left in before_request, then maybe we could get rid of before_request altogether? maybe could put 'global Inventory' to either the /checkout's 'def checkout' function, or to a global space like L59 so it's unbound to any function.
would have to make sure the Inventory info still gets set appropriately in the Additional Information.
global Inventory | ||
with sentry_sdk.configure_scope() as scope: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove this, I think it breaks functionality for setting the (email) user on the GET /tools endpoint. I think your get_tools
transaction stopped getting a user set after a certain time yesterday, here's the Discover query (note every other one here is from a pre-flight request so will never have it, but the final 2 are both missing it)
See that the current master branch still has 'email' being passed via request header for /tools, which depends on @app.before_request
parsing it
https://github.com/sentry-demos/tracing/blob/master/react/src/components/App.js#L120-L126
I know it means parsing info from both Headers and Body but I think this is normal, as user + auth info is often set in the Request Headers https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers and we need to accommodate GET requests.
But would this mean we want 'customerType' passed through request headers just so it will also be present on the getTools transaction? I'd rather not see customerType in headers. I like having any user Id/email/username (think auth) in the request headers but nothing more. Hmm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, lets pass (via payload) and set user the same way in both tools and checkout.
Looks like we will have to keep session-id as a header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndmanvar how are you proposing to accomplish, "set user the same way in both tools and checkout."?
If you mean use the payload fur user, then the /tools endpoint does not have access to the payload, because it's a GET request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah hmm, we shouldn't be adding payloads to GET request. we may have to use header. oversight on my part, and was trying to simplify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let you/Richard decide (:
@rpropst I think there are 2 options now
^ easiest/fastest fix.
Personally I like having the before_request code there and parse at least something (user header, anything), so it can serve as a demonstrable example if a customer has a question and the solution may involve this kind of hook. However, we're still showing parsing of headers/bodies in other endpoints, so that can work too as the example. The customer will understand the idea of a hook/middleware "on every request" if we advise them about it. |
This code adds the customer-type value to the header of the
/checkout
API call.