`undefined` userId considered in context

SimeonKostov commented 1 year ago

Describe the bug

In our React app, we had been updating the Unleash context's userId field in the following way:

useEffect(() => {
  updateContext({userId});
}, [userId])

until we noticed that there was an issue with the variant splitting for one of our feature flags - a flag with control and challenger variants, each with 50% weight.

Initially, the userId is undefined until an account is created, and seems like the undefined value is sent to the context as a string because the splitting issue is just for a single flag used before an account is created.

Steps to reproduce the bug

Create a feature flag with 2 variants
Assign 50% weight to each variant
set undefined to the userId context field
monitor how many times both variants have been assigned

Expected behavior

When a userId is undefined in the context, a sessionId should be used in order to distinguish the different users calling the proxy/server.

Logs, error output, etc.

No response

Screenshots

No response

Additional context

No response

Unleash version

unleashorg/unleash-proxy:v0.11.7

Subscription type

Pro

Hosting type

Hosted by Unleash

SDK information (language and version)

@unleash/proxy-client-react v3.5.1, unleash-proxy-client v2.4.2

thomasheartman commented 1 year ago

Hi, @SimeonKostov! Thanks for reporting this. But I'm not sure I understand exactly what the bug you are describing is. Could you elaborate?

You say that "there was an issue with the variant splitting for one of our feature flags". What was that issue? You later refer to it as the "splitting issue". What exactly is happening? Are you not seeing the split? Or is it "flip-flopping" between rerenders?

And in terms of the variant configuration, what have you set as the stickiness? Are you using the "default" stickiness?

Looking at it locally and some of the tests we've got (plus a few more I just added), things seem to be working as expected on my end 🤔 Looking forward to get some clarification on your end.

thomasheartman commented 1 year ago

Someone informed me that this probably comes from this community slack thread that has a lot more context, so I'm putting the link here for that case.

I don't know exactly what's happening here, but I tried reproducing the case you were seeing in the tests in #147, but it seems to work as expected.

Regarding undefined being stringified to "undefined": shouldn't you see it in the query parameters you send in that case? I also can't reproduce that in the tests in any way. Not ruling it out completely, but I'm not sure why it would behave that way outside the tests 🤔

SimeonKostov commented 1 year ago

@thomasheartman Sorry for the confusing description!

Someone informed me that this probably comes from this community slack thread that has a lot more context, so I'm putting the link here for that case.

True! The issue is related to this thread.

This was our case:

We used to update our Unleash context in the following way from the top-level component of our React app:
```
useEffect(() => {
updateContext({userId});
}, [userId])
```
We have a feature flag with control and challenger variants with a weight of 50%
The feature flag above is used on a step in our funnel where a user is not created yet and so the first time updateContext is called, should be called with {userId: undefined}
From our analytics, we noticed that there was a major difference between the leads that had seen the control and those that had seen the challenger - for the period 01.03-03.03 the numbers were 569 (control) vs. 84 (challenger)
After we noticed this difference, added a check to the place where we are updating the context (in the top-level component) and since that change, the numbers started to equalize.
```
useEffect(() => {
if (userId) {
updateContext({userId});
}
}, [userId])
```

Those observations made us think that there is a problem with sending an undefined userId. Hope this explain the case better!

Apologize in advance if this turns out to be a fake alarm!

thomasheartman commented 1 year ago

Thanks for the extra context! It's easier to have it here than in the slack thread, indeed!

You've already heard from several of the team members that yeah, those numbers look mighty skewed, so there's nothing new I can tell you there.

The feature flag above is used on a step in our funnel where a user is not created yet and so the first time updateContext is called, should be called with {userId: undefined}

So the first time it's called, it's called with an undefined userId, but later on, the ID should be defined right?

I know you already checked the query parameters in the thread, but could I ask you to double check that it does not send a userId parameter when you set userId to undefined? At least as far as I can understand, that could rule out the "undefined might be stringified" hypothesis. As I said, the tests I added in #147 do not add that query param to url.

I'm actually beginning to think that this might be something that's happening on the Proxy and not in the client. But I know you talked a bit about that in the thread too (with the 500 curl requests and all) 🤔

SimeonKostov commented 1 year ago

I know you already checked the query parameters in the thread, but could I ask you to double check that it does not send a userId parameter when you set userId to undefined? At least as far as I can understand, that could rule out the "undefined might be stringified" hypothesis. As I said, the tests I added in https://github.com/Unleash/unleash-proxy-client-js/pull/147 do not add that query param to url.

I am confirming that the userId parameter has not been sent while the userId is undefined.

I'm actually beginning to think that this might be something that's happening on the Proxy and not in the client. But I know you talked a bit about that in the thread too (with the 500 curl requests and all) 🤔

True, I made some curl requests, but I haven't added any context to the requests. Those tests seemed to work fine - always returning the same result

thomasheartman commented 1 year ago

I am confirming that the userId parameter has not been sent while the userId is undefined.

Thank you! 😄

True, I made some curl requests, but I haven't added any context to the requests. Those tests seemed to work fine - always returning the same result

Yeah, great. And I think it was suggested that remoteAddress might be the reason behind that.

Anyway, thanks for all the information! I don't think we can do anything about it this week, but I'll bring it up for next week's planning, so we'll probably do some more digging then.

thomasheartman commented 1 year ago

Hey, @SimeonKostov! So we've talked about this a little bit internally, but unfortunately we haven't been able to reproduce it. The issue seems serious, but because no one else has reported this, it seems probably that the issue is on your end.

Can you reproduce this with a completely clean Unleash setup running locally as well?

If you could provide us with a reproducible example, then we'd be more than happy to have a look at it, but without that, there's not much more we can do here, I'm afraid.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

thomasheartman commented 1 year ago

@sighphyre Didn't you find out what the cause of this was? Has that change propagated to the Proxy?

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Unleash / unleash-proxy-client-js