flagsmith-js-client icon indicating copy to clipboard operation
flagsmith-js-client copied to clipboard

feat: send client info to Flagsmith

Open tiagoapolo opened this issue 8 months ago • 7 comments
trafficstars

Ref: #2479

  • This pull request includes changes to the flagsmith-core.ts file to add support for appName and appVersion in the Flagsmith class.
  • Updated the getJSON method to include X-Customer-Application-Name and X-Customer-Application-Version headers if appName and appVersion are provided.
Screenshot 2025-03-04 at 15 05 54

tiagoapolo avatar Mar 04 '25 17:03 tiagoapolo

Do we want to make the new properties a part of the shared evaluation context interface?

CC @matthewelwell @kyle-ssg

khvn26 avatar Mar 04 '25 18:03 khvn26

Do we want to make the new properties a part of the shared evaluation context interface?

CC @matthewelwell @kyle-ssg

@khvn26 I'm not sure if these properties should be part of the evaluation context? Would we expect them to change?

matthewelwell avatar Mar 07 '25 13:03 matthewelwell

@matthewelwell We wouldn't during client runtime, but actually segmenting by app name and app version in evaluation could be desirable for the future?

khvn26 avatar Mar 07 '25 14:03 khvn26

@matthewelwell We wouldn't during client runtime, but actually segmenting by app name and app version in evaluation could be desirable for the future?

That makes sense, we could use it the future

tiagoapolo avatar Mar 10 '25 13:03 tiagoapolo

Ok, sounds good.

@khvn26 @tiagoapolo do you guys have an idea of the changes that are needed then?

matthewelwell avatar Mar 10 '25 16:03 matthewelwell

I've opened a PR to update the schema here: https://github.com/Flagsmith/flagsmith/pull/5203

After it's merged, we'll be able to regenerate evaluation-context.ts

khvn26 avatar Mar 10 '25 17:03 khvn26

The code itself looks fine, but I'm curious why we chose this approach instead of setting these attributes as traits. I can speculate as to why:

  • We don't have reserved trait names, so we can't guarantee there will not be collisions with existing customer traits. This is still a solvable problem, with some effort
  • Customers that track application attributes would always have to use the /identities endpoint (probably with transient traits), since /flags does not accept traits

My impression is that setting these attributes as traits is the better option; it has its challenges, but I don't see what we would gain by tracking them separate from traits. By using traits, every SDK (including OpenFeature) automatically supports this and we don't have to adapt clients in any way.

Another question of asking this would be - if customers want to target flags based on application name/version/platform/etc, they can do that right now with traits. What would they gain by using special headers instead? I'm sure I'm missing some context to be able to answer this question.

The goal is to track usage and using headers for that case is more appropriate. Also, we don't want to store that information in traits for future evaluation for now, that can come up in a new task/PR.

cc: @matthewelwell

tiagoapolo avatar Apr 02 '25 19:04 tiagoapolo

@rolodato I get your point. It would be critical when be used for traits to avoid any discontinuity in case of value change.

(I'm repeating also to confirm my understanding of the objective) In the context of gathering usage and tracking data, as long as they are informational it wouldn't bring any disruption of service to change the name of an application. Because we have the API key on which we could rely to track [project,environment], I see this as a way to group together requests/usage of a sub-application on a given environment, answering the question:

  • For my production environment, with 4 different applications using the same key, how is my usage distributed.

So with a name change, it would cause a spike change (from bullet to flagsmith) but the actionability of the underlying data would stay the same imo ("we need to decrease our usage in this one").

I agree with you though, as soon as this is used for traits and evaluation it will need to have a persistent key over time even with name changing etc

In the context of this PR I personally think it's not blocking and it's a base for future finetuning

Zaimwa9 avatar Apr 15 '25 11:04 Zaimwa9