[Proposal] Remove Get Provider from the Flag Evaluation API about spec

[justinabrahms]

Third option, which is how I would solve this in the current way is this:

// Startup
// Create
OpenFeature.setProvider(
    new PostHogProvider({
      apiKey: process.env.POSTHOG_API_KEY,
      personalApiKey: process.env.POSTHOG_PERSONAL_API_KEY,
    }),
 )
const client = OpenFeature.getClient('posthog', '1.0.0', {
    'service-name': 'my-service',
  })
 container.set<Client>(TYPES.OPENFEATURE_CLIENT, myOpenClientInstance)
 
// Controller 
const myOpenClientInstance = container.get<Client >(TYPES.OPENFEATURE_CLIENT)
client.addHook(new Hook() {
  before() {
     return new Context({PostHogProvider.GROUP_IDENTIFY_KEY: {
        groupType: POSTHOG_ATTRIBUTES.ORGANISATION,
        groupKey: organisationKey,
        properties: {
          name: organisationName,
          organisationId,
        },
      }))
  }
})

## The provider uses the `groupIdentify` value in the context if it exists in the context/hook hints.

 const isBetaFeatureEnabled = myOpenClientInstance.getBooleanDetails('my-beta-flag', false, {
    targetingKey,
    {
        [POSTHOG_ATTRIBUTES.ORGANISATION]: organisationId
    }
  })

Downside is there a risk that you fat-finger the group identify key such that it's not in the right format.

That said.. I think adding this back into the spec seems fine to me.

[toddbaert]

I think the use-cases were all associated with getting some basic data from the provider and the client, such as its name (used in the OTel hook, for instance). I propose adding a provider metadata concept as a field/accessor on the provider, that for now, will contain just the provider name. We can add additional metadata as required, so it's somewhat future-proof. We can expose this instead of the provider itself.

[toddbaert]

I've re-opened this because @weyert has brought up a use-case, and I think it's brings up a good point.

We decided to remove the provider accessor from the client object. I think this makes sense. The client is basically the primary interface for OpenFeature to the application author, so I don't want to pollute it. However, especially right now while we are still missing some features such as eventing and tracking, we may want to provide SOME access to the underlying provider, which might expose some of these from it's underlying SDK, or even provide a means of accessing that SDK.

I propose we continue NOT to expose the provider instance on the client, or in hooks, but that we DO have a means of retrieving it from the top-level API object, ie: OpenFeature.getProvider(). This will return the registered provider, which the author will have to cast to their particular implementation, thus allowing them to access the specific features of their underlying SDK, if their provider makes them available.

@justinabrahms I'm particularly interested in your take here.

cc @weyert @davejohnston @benjiro @ajhelsby

[justinabrahms]

Restated: as a user of the vendor agnostic library.. sometimes I need vendor specific functionality. To accomplish my goals, I want access to the provider in... what?

If you want it in your user code.. you can use the provider that you gave to the OF api singleton.

Is that not possible/easy?

[toddbaert]

If you want it in your user code.. you can use the provider that you gave to the OF api singleton.

Is that not possible/easy?

100%, this is true, and I would not be upset if we don't implement this, and simply recommend people do as you imply. However, I think it's quite reasonable that the area in code where an author wants such features from the underlying SDK, they may not have easy access to it (especially if they need access to the same particular instance). OpenFeature is always accessible because it's a global singleton, so the instance can be easily retrieved.

[justinabrahms]

Can you share the use case? I'm not opposed to it, but preference towards generally not going stuff. :)

…

On Tue, Jun 21, 2022, at 9:58 AM, Todd Baert wrote: > If you want it in your user code.. you can use the provider that you gave to the OF api singleton. > > Is that not possible/easy? > 100%, this is true, and I would not be upset if we don't implement this, and simply recommend people do as you imply. However, I think it's quite reasonable that the area in code where an author wants such features from the underlying SDK, they may not have easy access to it (especially if they need access to the same particular instance). OpenFeature is always accessible because it's a global singleton, so the instance can be easily retrieved. — Reply to this email directly, view it on GitHub <#84 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAA6DKLEYPHUDL5IPMS5ALVQHKC5ANCNFSM5XCWZFAA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[toddbaert]

I'll leave that to @weyert , since I think he brought the issue up. I believe it was to use a particular method on the PostHog SDK.

@justinabrahms I understand your desire to default to not adding this. If we don't hear anything from anyone else, I will re-close.

[weyert]

Can you share the use case? I'm not opposed to it, but preference towards generally not going stuff. :)
…

Yes, the use case is to be able to call identify and alias, groupIdentify, and captureEvent on the client. That's my use case so that I can identify user also from the backend and not only front-end code. Or prepare user's organisations by calling groupIdentify.

I think it's more convenient to have the provider manage the instance of the vendor client then having two keep track of both the OpenFeature client instance and the vendor client to be able to things like things. I think code like the following looks more convenient:

// Startup
// Create
OpenFeature.setProvider(
    new PostHogProvider({
      apiKey: process.env.POSTHOG_API_KEY,
      personalApiKey: process.env.POSTHOG_PERSONAL_API_KEY,
    }),
 )
const client = OpenFeature.getClient('posthog', '1.0.0', {
    'service-name': 'my-service',
  })
 container.set<Client>(TYPES.OPENFEATURE_CLIENT, myOpenClientInstance)
 
// Controller 
const myOpenClientInstance = container.get<Client >(TYPES.OPENFEATURE_CLIENT)
myOpenClientInstance.getVendorClient().groupIdentify({
        groupType: POSTHOG_ATTRIBUTES.ORGANISATION,
        groupKey: organisationKey,
        properties: {
          name: organisationName,
          organisationId,
        },
      })
 const isBetaFeatureEnabled = myOpenClientInstance.getBooleanDetails('my-beta-flag', false, {
    targetingKey,
    {
        [POSTHOG_ATTRIBUTES.ORGANISATION]: organisationId
    }
  })

instead of

// Startup
const posthogInstance = new PostHog(process.env.POSTHOG_API_KEY, {
      host:  'app.posthog.com',
      personalApiKey: process.env.POSTHOG_PERSONAL_API_KEY,
      featureFlagsPollingInterval: 1000,
    })

OpenFeature.setProvider(
    new PostHogProvider({
      apiKey: process.env.POSTHOG_API_KEY,
      personalApiKey: process.env.POSTHOG_PERSONAL_API_KEY,
    }),
 )
const client = OpenFeature.getClient('posthog', '1.0.0', {
    'service-name': 'my-service',
  })
 container.set<PostHog>(TYPES.POSTHOG_CLIENT, posthogInstance)
 container.set<Client>(TYPES.OPENFEATURE_CLIENT, client)

// Controller

// Use posthog
const posthogInstance = container.get<PostHog >(TYPES.POSTHOG_CLIENT)
posthogInstance.getVendorClient().groupIdentify({
        groupType: POSTHOG_ATTRIBUTES.ORGANISATION,
        groupKey: organisationKey,
        properties: {
          name: organisationName,
          organisationId,
        },
      })
 
 // Use OpenFeature Client
const myOpenClientInstance = container.get<Client >(TYPES.OPENFEATURE_CLIENT)
const isBetaFeatureEnabled = myOpenClientInstance.getBooleanDetails('my-beta-flag', false, {
    targetingKey,
    {
        [POSTHOG_ATTRIBUTES.ORGANISATION]: organisationId
    }
  })

As you can see in this second approach you need to maintain both a OpenFeature client and a Posthog client decide which one will be responsible for cleaning up / flushing queued data. Also in my opinion it looses the benefits of using OpenFeature as why not just use Posthog client instead?

I think offer access to the underlying vendor client would solve this problem and allows you OpenFeature to manage the instance and helps to have only one approach. In my case this groupIdentify function is underwater a special event that gets triggered so in theory once the event tracking is available it won't be needed.

I would consider it a special use case that is offered when really needed but is advised against :)

[toddbaert]

@weyert This is similar to what I though, thought I was proposing not having an ability to get the vendor client on the openfeature client, but just on the OpenFeature API object.

So just like there's a static/single OpenFeature.setProvider, there'd be an OpenFeature.getProvider. In this case, you wouldn't even need to do the lookup in your IoC container.

[weyert]

That would even be more convenient :D

[toddbaert]

I definitely like your strategy @justinabrahms , and @weyert, it might be worth your consideration for your use-case

I think though there's some standalone methods (specifically I'm thinking about things like track from split) which couldn't be reasonably baked into hooks.

[weyert]

Thanks for your idea @justinabrahms I will experiment with it that looks like a lovely approach I haven't thought off.

@toddbaert Yeah, I did start a proposal for the track: #102
Struggling a bit with the wording and the structure of the proposal itself but WIP. Open for help :)

[toddbaert]

So @weyert @justinabrahms ... do you think we should add this at this point?

Seeing that there's a hook way to do this, and that an author could always maintain their own access to the vendor client, I'm still not 100% sure.

[justinabrahms]

I'd like to put it in the beta target if we add it. It should allow us to get some real feedback on the usage of the thing while we play with this hooks option to see if it's worth going that way.

[toddbaert]

I will quickly bring this up in the community meeting, but I'm leaning towards closing this for now, and perhaps revisiting it later cc @weyert

[toddbaert]

I haven't heard otherwise, so I'm closing this for now, especially considering we'd like to get alpha SDKs out.