Should we remove Confidence.id.generate() and Confidence.id.criteria()?

[devinivy]

Confidence has two undocumented (but public) methods Confidence.id.generate() and Confidence.id.criteria(). Their purpose is to generate a guid and then use that guid to generate random criteria that can be used in the context of A/B testing. For example, Confidence.id.generate() could generate a guid to assign to a user's session, and that guid seeds random criteria values to obtain a random (but stable) configuration for that user. This way the user receives a random experience based on the confidence configuration, but the experience doesn't change for them over time.

This isn't documented, pal does not use confidence in this way (in the boilerplate), and I cannot find any issues related to it in github, so I wonder if we can safely part ways with this functionality in v6.

[Nargonath]

To be honest I've been neither aware of this feature nor have I needed it as of now. In that regard I don't mind if we remove it but there could be people using it. Let's see if we get other voices on this. I'll post it on Slack too to bring more awareness.

[damusix]

Same. Just looked at tests for and I can't see a use case for it in what I do. What was the original intention of this functionality?

[ethriel3695]

We use both Confidence.id.generate() and Confidence.id.criteria(abId) for the exact use case you describe above in the issue description

[kpdecker]

When we originally developed the lib, these methods were core to the entire goal of the project, at least for the use case for Walmart Mobile. This was also propagated on the @ethriel3695's org (hi!) but by myself, so they still likely count as one data point :)

There are some examples that do provide very light docs, but they are not in the place you would expect. https://github.com/hapipal/confidence/blob/master/examples/ab.js

As devil's advocate: What are the gains from removal? Are there performance or maintenance concerns around this code that would be solved by introducing a breaking change that invalidates the AB testing use case?

[devinivy]

What are the gains from removal? Are there performance or maintenance concerns around this code that would be solved by introducing a breaking change that invalidates the AB testing use case?

I don't see any serious performance or maintenance concerns. The gains would really just be cleanup at an opportune time, if there was code in the project that was no longer relevant for users. I had a hunch that could be the case, since it was omitted from the otherwise pretty detailed docs, I couldn't find any mention of it in GitHub correspondences, and when Eran opted to remove confidence from the hapijs org I didn't recall anyone else expressing interest/concern. We adopted it when hapijs was prepared to deprecate it because we use (and enjoy!) it in the boilerplate to build out server manifests, and I expect we'll continue to take confidence that general direction. All that said, if this is functionality people still use and care about, I don't see any burning need to nix it!

@kpdecker do you have this use-case too, or just a general proponent for holding onto it? @ethriel3695 are you using confidence v5 and intending to upgrade to future versions of confidence assuming this feature sticks around? If the answer to either of those are yes, then I expect there's a good chance it will stick around 👍

[devinivy]

I'll be releasing v6 with Confidence.id unchanged, but still leaving this open for further consideration and discussion. I hope to resolve both this issue and #106 in time for the following release, so we have some time to keep chatting. Don't want to rush anything :)

[ethriel3695]

@devinivy We utilize the functionality proposed for removal for ab testing as @kpdecker (hello back!!!) described and the docs link included is fairly similar to our current setup.

We have upgraded to Confidence v5.0 and plan on upgrading package versions and works very much appreciate this functionality to stay interact with future versions of Confidence 😁

[devinivy]

Cool, in that case I see no reason to shake things up 👍 thanks for showing up and chiming in!