-
Notifications
You must be signed in to change notification settings - Fork 14
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
remove errorneous error logging and db inserts in group experiment assign call #2148
remove errorneous error logging and db inserts in group experiment assign call #2148
Conversation
const experimentToExcludeIds = experimentToExclude.map((experiment) => experiment.id); | ||
|
||
// throw error user group not defined and add experiments which are excluded | ||
experimentToExclude.forEach(({ id, name }) => { |
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.
We should keep the logger and comment out the insertion in the database.
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.
it's not just the insert that is a problem, when i was testing this yesterday, the log and insert both are called for no reason for most users, regardless of if they have an invalid group or not.
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.
#2147 (comment) i can demonstrate this in my local in mtg
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.
will spawn a ticket to revisit this to figure out what the correct warnings we actually value for the scenarios where there really is working-group / group metadata issues.
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.
@VivekFitkariwala can i merge this? |
discussed in mtg, spawned a new issue
#2147