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

Adding customer-type tag to header for API #58

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rpropst
Copy link
Contributor

@rpropst rpropst commented Dec 18, 2020

This code adds the customer-type value to the header of the /checkout API call.

Screen Shot 2020-12-18 at 2 28 28 PM
Screen Shot 2020-12-18 at 2 29 12 PM

Copy link
Contributor

@thinkocapo thinkocapo left a 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
Copy link
Contributor

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.

@rpropst

global Inventory
with sentry_sdk.configure_scope() as scope:
Copy link
Contributor

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

@rpropst @ndmanvar

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

@ndmanvar ndmanvar Dec 22, 2020

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

Copy link
Contributor

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 (:

@thinkocapo
Copy link
Contributor

@rpropst I think there are 2 options now

  1. Keep all your code as it, but go into the GET/tools endpoint and manually parse out the email and setUser with the email (so form within the /tools endpoint and not the before_request middleware)

^ easiest/fastest fix.

  1. Put back the user's email into the Request Headers, and before_request middleware

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.

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

Successfully merging this pull request may close these issues.

3 participants