Skip to content

Conversation

@bgiori
Copy link
Collaborator

@bgiori bgiori commented Jul 25, 2021

Summary

  • Don't track exposure to fallback variants
  • Add tests for analytics provider exposure tracking

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: NO

@trashstack
Copy link

Why shouldn't we track exposure to fallback variants?

@bgiori
Copy link
Collaborator Author

bgiori commented Jul 26, 2021

Why shouldn't we track exposure to fallback variants?

I posted about this in #skylab-internal

My guess is that a customer will never use an exposure to a fallback metric as the exposure metric in an experiment. Therefore it is not useful to send.

For debugging, tracking exposures to fallbacks would be useful. Nirmal brought up that we could send a different metric in this case (e.g. [Experiment] Fallback Exposure).

return { value: undefined };
}
const sourceVariant = this.sourceVariants()[key];
if (sourceVariant?.value) {

Choose a reason for hiding this comment

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

Why not if (sourceVariant) {?

I had to check but we do currently require a value so this existing code is correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically, sourceVariant could be non-null with a null value, although this is definitely not the regular case and I'd doubt it'd ever happen. That being said, a null value means an invalid variant, which we do not want to track.

Copy link

@trashstack trashstack left a comment

Choose a reason for hiding this comment

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

lgtm

@bgiori bgiori merged commit bd4917f into main Jul 28, 2021
@bgiori bgiori deleted the bgiori/dont-track-fallback-exposure branch July 28, 2021 16:33
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