-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fixed an issue where identity errors were masking other errors. Changed identity errors to warnings. #1086
Conversation
…ed identity errors to warnings.
@@ -24,18 +24,16 @@ export default ({ doesIdentityCookieExist, orgId }) => { | |||
} else { | |||
// This logic assumes that the code setting the cookie is working as expected and that | |||
// the cookie was missing from the response. | |||
const noIdentityCookieError = new Error( | |||
`An identity was not set properly. Please verify that the org ID ${orgId} configured in Alloy matches the org ID specified in the edge configuration.` | |||
logger.warn( |
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 like that we're providing multiple reasons for this error to occur. I would love to get us to a place where we don't check for this cookie at all. There are multiple Identity related conversations happening soon that might help us get there.
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 think this error is masking the true error that is occurring. I'm hoping that if we release this, we'll get fewer reports of this error, because the cause of the problem will be apparent.
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.
Do we have any visibility/logging into the "true error"
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.
The true error will be the one that rejects the promise so it should be visible in the console. I suspect that it will point to customer's code and we won't hear about it.
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 like this approach. Would love to see if you can run it on the customer's site where you were able to repro this issue.
@@ -24,18 +24,16 @@ export default ({ doesIdentityCookieExist, orgId }) => { | |||
} else { | |||
// This logic assumes that the code setting the cookie is working as expected and that | |||
// the cookie was missing from the response. | |||
const noIdentityCookieError = new Error( | |||
`An identity was not set properly. Please verify that the org ID ${orgId} configured in Alloy matches the org ID specified in the edge configuration.` | |||
logger.warn( |
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.
Do we have any visibility/logging into the "true error"
Description
I wasn't able to reproduce the error directly using just the library or launch library. Through investigation of the code, and running a modified version of the library, I found that when there is an error while processing the response from Konductor, the identity error was masking the original error. I suspect that this error is showing up a lot for customers because it is masking other errors. I've changed the code so that when the identity cookie isn't found, it logs a warning, but doesn't fail the command. The command will fail though if there are other errors that may have caused the identity problem.
Related Issue
https://jira.corp.adobe.com/browse/PDCL-10719
https://jira.corp.adobe.com/browse/PDCL-11026
https://jira.corp.adobe.com/browse/PDCL-11278
Motivation and Context
Screenshots (if appropriate):
Types of changes
Checklist: