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

Mitigate undefined errors in PValueColumn #1025

Merged
merged 1 commit into from
Feb 24, 2023
Merged

Conversation

lukesonnet
Copy link
Collaborator

Features and Changes

When building the ResultsTable we may pass some default stats if a row is missing for a variation for some reason. We do:

  {variations.map((v, i) => {
    const stats = row.variations[i] || {
      value: 0,
      cr: 0,
      users: 0,
    };
    .BUILD SOME THINGS.
    }

Because we don't populate expected or pValue, if this code were to execute for some edge case, then the PValueColumn code which expects defined values for those fields will throw an error.

This PR fixes that by letting them be undefined and setting them to some default values if needed. The values themselves shouldn't matter since users should be 0 and therefore there should not be enough data to generate anything other than an empty <td>.

I think this is causing a customer reported error and I think that this patch will at least fix the immediate issue.

We may prefer to figure out why this edge case is being created at all and more directly solve the issue, but this may be sufficient to prevent the front-end from throwing errors that lock the user out of the experiment page altogether.

@github-actions
Copy link

Your preview environment pr-1025-bttf has been deployed.

Preview environment endpoints are available at:

? expected < 0
: expected > 0;
const pValue: number = stats.pValue ?? 1;
const statSig = pValue < pValueThreshold;
Copy link
Contributor

Choose a reason for hiding this comment

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

This addresses the problem, however I'm wondering if it might be better to exit early / render nothing if the above values are missing? Ultimately will defer to your opinion on this but curious to hear what you think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I was wondering the same thing, but decided to mostly ape the logic in ChanceToWinColumn. I can test both with my broken MongoDB and see what seems better.

@lukesonnet lukesonnet marked this pull request as ready for review February 24, 2023 16:22
@lukesonnet lukesonnet merged commit 1780973 into main Feb 24, 2023
@lukesonnet lukesonnet deleted the lsonnet/patch_pvalue branch February 24, 2023 16:28
Auz pushed a commit that referenced this pull request Mar 7, 2023
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