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

Adjust SLAS private proxy logging #1928

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

vcua-mobify
Copy link
Contributor

This fixes a small bug in our logging.

Currently, when a private client request sent to SLAS returns an HTTP > 400, we are swallowing the error to print out a message about enabling the private client proxy.

This adjusts the logger so that the correct SLAS error message is surfaced in the logs.

Example message:

pwa-kit-runtime._SlasPrivateClientProxy ERROR Failed to proxy SLAS Private Client request to http://localhost:3000/mobify/slas/private/shopper/auth/v1/organizations/f_ecom_zzrf_001/oauth2/token - HTTP 401
                                {
  "status_code" : "401 UNAUTHORIZED",
  "message" : "incorrect client type"
} {"statusCode":401,"protocol":"http","originalUrl":"/mobify/slas/private/shopper/auth/v1/organizations/f_ecom_zzrf_001/oauth2/token"}

Testing this change:

  • Check out this change
  • Enable SLAS private client mode (there are switches in the app's _app-config and ssr.js)
  • Start the app with some obviously incorrect client secret ie. PWA_KIT_SLAS_CLIENT_SECRET=someSecret npm start
  • Check the outputted logs and verify that the error message from SLAS is not swallowed.

@vcua-mobify vcua-mobify requested a review from a team as a code owner July 23, 2024 18:53
@vcua-mobify vcua-mobify requested review from adamraya and kevinxh July 23, 2024 18:54
additionalProperties: {
protocol: req.protocol,
originalUrl: req.originalUrl
proxyRes.on('data', (data) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an concern here about the following:

  1. Are we sure that the data even is always called? e.g. can their be a failure that doesn't have a body on it?
  2. Is there the potential to have errors if there is similar code downstream that is trying to access the data event in the same way, possibly resulting in the downstream callback not firing?
  3. In question 1 above we wouldn't make any logging calls if there is a scenario where the data event is not fired. Do you think it makes sense that we have a logger call outside of this data callback?
  4. Do we know that the data is going to be the full message, e.g. does the data event only ever get called once? See this example of using the data event.. maybe we need to do something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also looks like we are losing all the setup errors. Do we not find these valuable? Could we use those as a way to help customers that are misconfigured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions @bendvc

  1. All responses from SLAS should have a body, even the HTTP 500s. Example 400 and 500 responses are shown in the SLAS docs
  2. Admittedly, I am not the most familiar with the data event - I just wanted a way to log the error message returned by the SLAS response in the body. So I cannot say if it will have any downstream effects or not.
  3. I've thought about having a logger outside of the data event but opted to consolidate everything into a single log entry for easier searching in Log Center
  4. I can give that a try.
  5. I feel that the setup errors here are redundant since we already return setup related errors elsewhere (see _handleMissingSlasPrivateEnvVar) and currently this message is misleading since we are outputting it on every instance where SLAS returns a 400 or 500 and swallowing the original message.

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.

2 participants