statsig-io / go-sdk

A golang SDK for interfacing with Statsig Feature Gates, Dynamic Configs, and A/B Experiments
ISC License
8 stars 10 forks source link

download_config_specs with has_updates: false resets the lastSyncTime #18

Closed andrehp closed 1 year ago

andrehp commented 1 year ago

Hello!

I was reading the SDK for Go to understand how it works and implement at least parts of it for our language of choice (Rust) and noticed a potential performance bug for the go implementation.

When fetching the config specs, the lastSyncTime is updated to the value from the response here, but this value is not sent by the API when has_updates:false. This probably causes the lastSyncTime to reset to 0 and cause the following request to download_config_specs to make the request with SinceTime: 0.

I'm not sure if not returning time when has_updates: false is the expected behavior, so I'm not sure if this should be handled by the client or if the fix should be done to the API.

vijaye-statsig commented 1 year ago

@tore-statsig , @daniel-statsig

tore-statsig commented 1 year ago

Rust you say? @daniel-statsig started one a while back but we havent had time to build that out further.

Nice find - theres no real harm here, the sinceTime is a bit of an optimization to not fetch all configurations every time we sync. But its definitely not working in this case

andrehp commented 1 year ago

Yes, we use mostly rust for our services, the company I work for is Rei do Pitaco. Right now I'm finishing the basic flow for feature gates, my idea for now is to handle our most common usecases and delegate the ones not implemented yet to a direct request to the API.

There seems to be no harm there, just some extra processing because of the reset of the flag. I just wanted to confirm if this was working as intended in the API.

daniel-statsig commented 1 year ago

Hey @andrehp, thanks for reporting this. We will fix this and push and update.

Regarding Rust. I have just pushed what I have to the public repo (https://github.com/statsig-io/rust-sdk). I believe I was fairly close to having the check_gate functionality working, but was stuck on trying to get the static Statsig interface to work nicely with the Rust borrow system.

Maybe this will help you in your implementation. Let me know if there is anything I can do to help you. As Tore said, we don't have resourcing to prioritize building an official SDK right now, but it's still something we are interested in.

andrehp commented 1 year ago

Thanks for the reference @daniel-statsig ! I will certainly take a look at the implementation. Once we have something more complete I believe we will be able to share the result of our efforts as well.

andrehp commented 1 year ago

Something else I noticed is that there might be a race condition when flushing the logs.

Here the events slice might be cleared before the sendEvents method is actually executed:

func (l *logger) flushInternal(closing bool) {
    if closing {
        l.tick.Stop()
    }
    if len(l.events) == 0 {
        return
    }

    if closing {
        l.sendEvents(l.events)
    } else {
        go l.sendEvents(l.events)
    }

    l.events = make([]interface{}, 0)
}
andrehp commented 1 year ago

As promised, here is our current client for statsig in rust: https://github.com/reidopitaco/statsig-rs

It fetches the data from statsig and tries to use it for the feature gates, but when it doesn't know how to handle them it delegates to an http call to the API. It also supports dynamic configs, but right now it only gets the data from the API.

vijaye-statsig commented 1 year ago

Amazing! Thank you, @andrehp