open-feature / playground

OpenFeature SDK demos and experimentation
https://openfeature.dev
Apache License 2.0
50 stars 25 forks source link

Support system properties in context #6

Closed beeme1mr closed 2 years ago

beeme1mr commented 2 years ago

It would be nice to be able to define system properties that are merged with context before a flag evaluation. This could include information such as the version of the application, OS, data center location, etc. Client-side SDKs could set context such as screen size, browser, OS, etc.

Perhaps it could look like this:

const { openfeature } = import "@openfeature/openfeature-js";

openfeature.registerGlobalContext({
  "cloud.region": "us-east-1"
})

const client = openfeature.getClient("example");

(async () => {
  // Global and local context would merge before flag evaluation. Locally set
  // context could override global context in case of a collision.
  const enabled = await client.isEnabled("test-feature", { userId: "test-user" });
})()

The property name cloud.region comes from OTels semantic convention.

justinabrahms commented 2 years ago

There's global context, but also per-request context that can be set by the system. An example from our flag system includes: what region are people accessing? (global context) & is this request coming from an internal IP? (per request context).

In both cases, our internal tooling sets both in the context. Forgive the weird java-isms.

const { openfeature } = import "@openfeature/openfeature-js";

openfeature.registerGlobalContext({
  "cloud.region": "us-east-1"
})

const client = openfeature.getClient("example");

/* somewhere in the web framework */
client.registerRequestContext({ ip_is_internal: logic() })
/* end framework */

(async () => {
  const enabled = await client.isEnabled("test-feature", { userId: "test-user" });
})()

It's possible that requestcontext needs to take in some sort of request-id, but we don't really want to have to pass in that request id to the isEnabled call below.

moredip commented 2 years ago

I wonder if a more general version of "per-request context" would be some sort of scoped context, similar to how spans work in otel. I could enter a "flagging context" at the start of a request, and then at any time add attributes to the current context, and any flag evaluations would automatically merge in those attributes until I exited the flagging context at the end of a request.

Maybe I'm over-engineering this, but it feels like a nice general purpose mechanism that would enable both the static "system properties" that @beeme1mr is describing as well as the per-request context that @justinabrahms is describing.

justinabrahms commented 2 years ago

@moredip Seems fine to me. I'd prefer there be a cap on the number of levels of "flag context" we allow to begin with. Something like 3: system, request, instance. In the above example I provided, system would be cloud.region, request would be ip_is_internal, and instance would be userId.

moredip commented 2 years ago

Maybe there's a generic lower-level API that allows arbitrary nesting, but also a higher-level API that builds on top of that with some named scopes - e.g. system (or global), and request?

I'm not sure of the pros and cons of having that lower-level API a defined part of a spec, vs an implementation detail that might be surfaced in a future version of the spec.

beeme1mr commented 2 years ago

@justinabrahms, I completely agree that request scoped context should be added. Do you have a proposal for how it could work in node? We might be able to use OTel's context or something like node-continuation-local-storage. It's an easier problem to solve in other languages.

moredip commented 2 years ago

FWIW, otel's ContextManager seems to use async_hooks in node, and zone-js in browser-land.

Using ContextManager directly seems pretty tempting. The API is a great fit AFAICT.

beeme1mr commented 2 years ago

@moredip, this is a good place to explore new concepts. Feel free to open a PR so we can discuss it further.

Something like this could be interesting too:

const client = openfeature.getClient("example");

(async () => {
  const testFeature= await client.getVariation("test-feature", { userId: "test-user" });

 if (testFeature.isEnabled()) {
   testFeature.sendEvent("New feature")
  //  Execute new feature
 } else {
   testFeature.sendEvent("Old feature")
  // Execute old feature
 }

  testFeature.end();
})()

This may be overengineered too but it would allow you to see the impact of the feature with OTel. Currently, you would see a span representing the flag evaluation but you have less insight into the downstream impact it had.

justinabrahms commented 2 years ago

@beeme1mr I generally like the "pass arguments to functions with the things they need" approach. I'd think that the answer would be framework dependent.

Looking at https://koajs.com/, I think we might end up with something like:

const Koa = require('koa');
const app = new Koa();
const { openfeature } = require("@openfeature/openfeature-js");

openfeature.registerGlobalContext({
  "cloud.region": "us-east-1"
})

// add request-scoped variables
app.use(async (ctx, next) => {
  ctx.ffClient = openfeature.getClient('example');
  ctx.ffClient.requestContext({ internal_ip: is_internal(ctx.ip) })
  await next();
});

// response
app.use(async ctx => {
  const greeting = ctx.ffClient.getVariation("greeting", { userId: "test-user" }, "Hello");
  ctx.body = greeting + ' World';
});

app.listen(3000);
beeme1mr commented 2 years ago

@justinabrahms, got it, thanks. I think the method requestContext in the above example is a bit misleading. Perhaps we could call it client scoped context. Here's an example of what it could look like:

const Koa = require('koa');
const app = new Koa();
const { openfeature } = require("@openfeature/openfeature-js");

openfeature.registerGlobalContext({
  "cloud.region": "us-east-1"
})

// add request-scoped variables
app.use(async (ctx, next) => {
  ctx.ffClient = openfeature.getClient('example', { internal_ip: is_internal(ctx.ip) });
  await next();
});

// response
app.use(async ctx => {
  const greeting = ctx.ffClient.getVariation("greeting", { userId: "test-user" }, "Hello");
  ctx.body = greeting + ' World';
});

app.listen(3000);

We could still introduce request scoped context but it would have to span multiple clients automatically using something like async_hooks.

The layer of context could be: global < client < request < local

As context merging could be complex, it would be important to expose debugging information. That way, a developer could understand how a partially property was set.

toddbaert commented 2 years ago

I think we should be as flexible and as least prescriptive here as possible. Dependency injection frameworks in various languages provide already well-known and defined tools for this sort of thing, like property/constructor injection, and factory functions. These might provide a better place for the consumer to define their own "context override/merge" strategy if they are interested in such things. I think our primary goal should be to define an intuitive interface for "plugging in" such a context, as flexibly and generically as possible. Though I agree things like async-local-storage and thread-local-storage could provider other interesting mechanisms of context-propagation and injection, especially into third party libraries the Application Author doesn't have direct control over.

The term "RequestContext" seems to imply HTTP, but we don't know for certain that will be the transport method involved - maybe openfeature could be used in the context of a persistent connection such as a web-socket? In such a use-case "RequestContext" seems like a misnomer.

I think that providing interfaces that expose "hooks" within the flag evaluation process wherein users can provide arbitrary behaviors (functions) and data (context) is a good means of doing this sort of thing.

beeme1mr commented 2 years ago

Closing in favor of the issue in the spec.