launchdarkly / react-client-sdk

LaunchDarkly Client-side SDK for React.js
Other
81 stars 67 forks source link

Update useFlags to support analytics events #122

Closed pdrummond closed 1 year ago

pdrummond commented 2 years ago

Is your feature request related to a problem? Please describe.

In order to get the Launch Darkly dashboard to correctly report when flags are evaluated ("Evaluated xxx mins ago" on the flag page as well as the Insights tab for particular flags), we currently can't use your useFlags hook. We have to use useLDClient then invoke ldClient.variation which is more awkward and we have to always make sure we don't accidentally choose the useFlag hook because the documentation doesn't currently make it clear that it doesn't support analytics events, so it's bound to be the first choice for most, and it's very confusing as to why we can't use the default.

I know you can optionally pass in flags during initialisation which will invoke variation under the hood but that won't work for us either because it is evaluating flags at initialisation time rather than when the flags are actually evaluated.

Describe the solution you'd like

Ideally, update useFlags to support analytics events somehow or introduce a new hook and make it clear which one needs to be used to support analytics events being generated at time of evaluation rather than at time of initialisation.

Describe alternatives you've considered

Firstly, we always have to make sure the initialisation of LD uses sendEventsOnlyForVariation (which should be the default really as I don't see why anyone would want to report that flags are evaluated at initialisation time because that's not when they are evaluated - that's just when they are fetched).

To evaluate flags correctly, we've tried using useLDClient but it's not very convenient so we've ended up with a custom version of useFlags that simply exposes a function that invokes ldClient.variation under the hood which seems to be working well for us for now, but it would be much less confusing if we could use the official hook provided by this library rather than custom workarounds.

louis-launchdarkly commented 2 years ago

Hello @pdrummond, thank you for reaching out to us. We have discussed this request and here is the reason that we cannot couple the analytics with the default useFlags hook.

useFlags is backed by useContext, which will trigger when the context value change (https://reactjs.org/docs/hooks-reference.html#usecontext). And useContext is the means to expose a state that is accessible by many components. There can be many calls to useFlags in the page component tree, but if a flag value is getting used multiple times on a single page, it doesn't make sense to send an analytics event every time when useFlags is getting called. This is also not counting the case when component re-rendering causes the code to call useFlags again.

That is why useFlags decouples from analytics events when useFlags is evaluated, as that will likely create a lot of noise in your analytics data. The workaround, while it is not the most convenient, means that you are explicitly controlling when to send an event and will get a much more accurate analytics picture.

pdrummond commented 2 years ago

Thanks for the explanation @louis-launchdarkly but we aren't expecting the analytics to be super accurate in terms of the precise number of evaluations so if it reports multiples due to re-renders, I think that would be okay? 🤔

All we need is the following:

  1. For the "Evaluated XX ago" feature to work properly
  2. For the Flag Status feature to work properly

I don't think sending multiple analytics events due to re-renders would break these would it?

We need these to work mainly so we can identify inactive flags to be removed. That is our main concern at the moment as we have too many old flags and LD is telling us they are all active when we know they aren't!

It sounds like these two basic features of the LD dashboard are not supported by the React SDK by default. Is that true? If I understand correctly, the only way to support these two features is to use ldClient.variation() (potentially within useEffect so it only captures analytics when the page loads) but that is not documented anywhere so it's still unclear to me if this is the recommended solution.

wyattanderson commented 2 years ago

Firstly, we always have to make sure the initialisation of LD uses sendEventsOnlyForVariation (which should be the default really as I don't see why anyone would want to report that flags are evaluated at initialisation time because that's not when they are evaluated - that's just when they are fetched).

I agree with the suggestion here that sendEventsOnlyForVariation should be the default. I'm working on a new integration of the LaunchDarkly JS/React APIs into our applications and was very surprised to see that by default, an analytics event would be registered for every single flag when the <LDProvider> renders. Perhaps it's an oversight, but this seems to completely undermine core LaunchDarkly product functionality.

Ideally, I'd like to see what @pdrummond already mentioned: a useFlags hook that sends analytics events as one might expect (i.e. only when specific flags are actually accessed), or a separate hook that fetches an individual flag and sends analytics events appropriately. Right now, the useFlags hook creates a pretty big footgun, and personally, I think in its current form it should be removed or should have its pitfalls documented.

One option that might be worth exploring (and that we may explore internally) is returning an object from a useFlags-like hook that records analytics events on property access, either via a get accessor descriptor or via a Proxy.

If a flag value is getting used multiple times on a single page, it doesn't make sense to send an analytics event every time when useFlags is getting called. This is also not counting the case when component re-rendering causes the code to call useFlags again.

Wouldn't the JS SDK option allowFrequentDuplicateEvents address this problem? Documentation:

Whether or not to send an analytics event for a flag evaluation even if the same flag was evaluated with the same value within the last five minutes.

By default, this is false (duplicate events within five minutes will be dropped).

XieX commented 1 year ago

@wyattanderson:

I agree with the suggestion here that sendEventsOnlyForVariation should be the default.

Me too.

Perhaps it's an oversight, but this seems to completely undermine core LaunchDarkly product functionality.

It is and it does.

One option that might be worth exploring ... is returning an object ... that records analytics events on property access... via a Proxy

This is indeed worth exploring ;)

Wouldn't the JS SDK option allowFrequentDuplicateEvents address this problem?

Yes, it should 👍

@pdrummond:

It sounds like these two basic features of the LD dashboard are not supported by the React SDK by default. Is that true?

Regrettably that is true, at present.

[T]he only way to support these two features is to use ldClient.variation() (potentially within useEffect so it only captures analytics when the page loads)

Yep! With the sendEventsOnlyForVariation: true option. I don't know about "recommended", but I'd do something like this:

import { useEffect } from 'react'
import { withLDProvider, useLDClient, useFlags } from 'launchdarkly-react-client-sdk'

function App() {
  const { devTestFlag } = useFlags()
  const ldClient = useLDClient()

  useEffect(() => {
    // hack to send evaluation event for this flag once per component mount
    // note we ignore the return value because we want to use
    // the value from `useFlags` so we get rerenders when flag is updated
    ldClient?.variation('dev-test-flag') // <-- need to use un-camel-cased key
  }, [ldClient])

  return (
    <div>
      {`Flag is ${devTestFlag}`}
    </div>
  )
}

export default withLDProvider({
  clientSideID: 'd3adbeefcaf3deadb33fcafe',
  options: {
    // prevents evaluation events for all flags on app load
    sendEventsOnlyForVariation: true
  }
})(App)

The benefit of this approach is that if there were an update to the React SDK that set sendEventsOnlyForVariation: true by default and sent events when a flag in useFlags were accessed, this code would be forwards compatible, and the sendEventsOnlyForVariation option and useEffect would just become redundant code that you could clean up at your leisure.

XieX commented 1 year ago

A fix for this went out in 2.27.0, take a look and see if it's working more as expected now.

kinyoklion commented 1 year ago

Closing issue as it was fixed in 2.27.0. If something new comes up we can open another issue.

BrianTsGit commented 1 month ago

@louis-launchdarkly

There can be many calls to useFlags in the page component tree, but if a flag value is getting used multiple times on a single page, it doesn't make sense to send an analytics event every time when useFlags is getting called. This is also not counting the case when component re-rendering causes the code to call useFlags again.

I am currently experiencing this very behavior Louis pointed out a while back. By nature of just my component re-rendering (due to some user input and update to some view I'm updating), the flag gets evaluated again. Maybe its a personal preference but ideally I would like an accurate reading of how many evaluations a flag receives and not a conflated value.

I did see in a later comment that there was a option allowFrequentDuplicateEvents that would ideally prevent this behavior but it has since been deprecated. Is there a way today to stop frequent duplicate events, or a pattern at a higher level to stop this unwanted behavior?