Improving /setuid

[dbemiller]

This fixes two issues. I grouped them into the same PR because the code overlapped.

1. Add metrics for counting calls to /setuid, separated by bidder.
2. Re-sync users to audienceNetwork if we were told that their UID was 0.

The point of (2) is to circumvent around some questionable behavior from the Facebook user sync servers. Currently, they redirect to/setuid with a sentinel value of "0" if the user isn't logged into Facebook at the time. This is (at least partially) responsible for some really bad user sync rates, because prebid-server saves those 0s, considers the user to be synced, and then stops trying to sync later.

In reality, 0 is an invalid user ID... but eventually, we'd expect the user to log into Facebook, so that the audience network can sync with their real UID.

So... this PR also consolidates all UserSync Sets behind a TrySync() function, which it then implements to reject "0" IDs for audienceNetwork.

[mkendall07]

Looks ok to me. Not sure about needing an interface for cookie though.

[mkendall07]

Why do we need use an interface here? The type to me has to be tied to a concrete cookie as I'm not sure what else we could replace it with given the client/server interactions.

[dbemiller]

Good/fair question.

I really just wanted the compiler to guarantee that all the other PBS code was going through the setter methods. I don't want the next guy who refactors an endpoint to have to worry about the fact that facebook IDs shouldn't be 0.

So... I tried to make UIDs private, and add a the setter method to the PBSCookie. But it turns out that Golang's JSON marshal & unmarshal don't work on private variables.

This was the next best thing I could come up with. UIDs can stay public, butcookieImpl will be private. The interface just exists for the public API.

[dbemiller]

I guess the tradeoff here is...

If I have an interface, I can make the UIDs property private to the rest of the code. Without an interface, I'd leave a comment saying "// This is only public for JSON serialization. Don't use it directly. Call TrySync instead."

I generally favor using the build system rather than comments to enforce things when possible... but there is some overhead, and I've been known to take it too far. Let me know if you think it's worth removing.

[nhedley]

Is it possible instead of having the interface to create a struct that is public just for the purpose of JSON serialization? Something like what is described here (https://stackoverflow.com/questions/28035784/golang-marshal-unmarshal-json-with-both-exported-and-un-exported-fields).

Would that allow the uuids field to be private still without the need for the interface?

[dbemiller]

That looks like a viable option. I kinda want to compare the code side-by-side... so I'll stick it in another branch.

[dbemiller]

Implemented this in #106.

The two look pretty similar in complexity to me... but I think I give a slight edge to the concrete struct. This is how I'm thinking about it:

Concrete struct benefits:

• nil-safety is possible (though it requires some extra branchy code).
• nil is much clearer to mobile apps, since there's no such thing as a UserSync there.

Interface benefits:

• No custom JSON code, so there's less room for bugs

In this case I think the confusion caused by a UserSync map on mobile requests is probably the most important part.... so gonna go with that one.

[nhedley]

Do we need an empty SyncMap on mobile app requests for everything to work? Cookie sync doesn’t make sense on mobile app, so having an empty sync map could be confusing.

[dbemiller]

The short answer is... I'm not sure. I definitely agree that it's confusing.

On the other hand... func (req *PBSRequest) GetUserID(BidderCode string) surfaced some interesting unit test failures. Apparently in Go any reads on a nil map are treated as if that map were empty. So pbsReq.UIDs['key'] will just return nil if UIDs is undefined, but req.SyncMap.GetUID() will cause panic. Stashing an empty SyncMap on here makes reads behave nicely.

I don't know offhand whether any of the other PBS code attempts to read cookiesyncs inside the "mobile" code branches... but as a general rule, ab object should be equally usable regardless of how its constructed.

I think this lends some extra weight to your suggestion here. If the methods are defined on a struct, then I can define how they behave on a nil. Interfaces don't give that flexibility.