feat!: v5 #151

rolodato · 2025-03-22T00:56:24Z

Breaking changes

Removed support for transient traits and transient evaluations

Traits are key-value pairs, so they are most naturally represented by a map[string]any. However, because each individual transient can be transient or not, we have to introduce a new Trait type to account for this, so we lose the simplicity and interoperability of using plain maps. The same applies for identifiers, which could be represented by a native string but instead we have to use an Identity struct or awkwardly pass around booleans.

To me, this tradeoff is not worth it, because this SDK will almost always be used with local evaluation, i.e. all evaluation is transient anyway. Transient identities can still be used with an empty identifier.

More thoughts on transient traits

I think the concept of transient traits and identities is flawed and should be replaced with something simpler. I also think this is the reason why supporting transient traits in SDKs causes the code to be so cumbersome and awkward.

An evaluation context is some data that should know nothing about when or how it's being evaluated. The fact that Flagsmith can sometimes persist context data as a side effect of remote evaluation does not belong as part of the evaluation context. These are orthogonal concerns that should be kept separate.

For example, the /identities API could accept an allowlist and denylist of trait keys to persist, separate from marking each individual trait as transient or not. This avoids tying trait data and whether to store this data as part of the same entity.

Simplified evaluation context types

NewEvaluationContext and NewTransientEvaluationContext are now the only ways to obtain EvaluationContext instances. All other related types such as Identity, IdentityEvaluationContext, TraitEvaluationContext andEnvironmentEvaluationContext are now removed.

TraitKey and TraitValue are renamed to Key and Value respectively in the Trait struct. Using map[string]interface{} to represent traits is preferred.

Removed `WithOfflineHandler` in favour of `WithOfflineEnvironment`

Previously, Client required two options for using offline mode: WithOfflineMode and WithOfflineEnvironment, which restricted with other SDK options could be used. Also, the idea of an offline handler was misleading, because the handler was only ever called once during SDK initialisation, and never again during the SDK lifecycle.

Now, we've added WithOfflineEnvironment which accepts a constructed EnvironmentModel instead. NewLocalFileHandler was removed in favour of ReadEnvironmentFile, which reads a file into an EnvironmentModel.

We also added a SetEnvironment method which lets consumers of the SDK update their offline/local environment whenever they want, instead of whenever the SDK decides to.

Simplified error types

Removed FlagsmithClientError since it only wraps string messages and is not used in any error handlers.

Renamed FlagmithAPIError to APIError. Removed Msg, ResponseStatusCode and ResponseStatus. Added APIError.Response, which should be used instead.

NewClient no longer panics

NewClient can now return an error and does not panic. Added MustNewClient, which can panic.

Removed custom logger interface

WithLogger now accepts a *slog.Logger instead of a custom logger implementation.

New features

The SDK now uses slog for all its logging. All network requests, responses, durations, failures and errors can now be debug logged. Many logs were improved with useful context and more helpful messages.

WithDefaultFlagHandler now automatically sets Flag.IsDefault and Flag.FeatureName. Previously, handlers would have to do this manually, which is unnecssary.

When using the SDK offline, the local environment can be now updated at any time by calling Client.SetEnvironment instead of only once after NewClient is called.

Bug fixes

Fixed environments being unnecessarily fetched when the realtime service responds immediately with the latest environment timestamp after a connection starts.

Other changes

GetEnvironmentFlags and GetIdentityFlags are no longer deprecated. These are useful aliases for the most common use cases across all SDKs.

The current environment state is now stored using a mutex and a sync.Map instead of atomic.Value references stored in context values. This makes concurrency bugs less likely and is more idiomatic.

EvaluationContext is now entirely defined in this package, and not code-generated from a common schema.

All tests now print debug logs by default.

khvn26

I like the overall approach and didn't find any immediate issues having read through the code. That said, I expect the ability to provide transient traits/identities to be restored in a separate PR, preferably ahead of next release — we can discuss the exact approach offline.

state.go

gagantrivedi

I have gone through some of the code, but not all of it yet.

client.go

analytics.go

gagantrivedi · 2025-03-25T11:16:39Z

evaluationcontext.go

 }

-type EnvironmentEvaluationContext struct {


Why did we get rid of this?

IMO it doesn't provide any value to the SDK, and we should encourage customers to create evaluation contexts using our functions such as NewEvaluationContext. I can't see any reason why it should have been here in the first place. I'm happy to add it back if we can find a good reason to.

It was used to dynamically pass environment key to get flags etc

That can and should be passed as the argument to GetFlags. I really would like to not have the environment be mutated by SDK users.

In general, the evaluation context for server-side SDKs should not be mutable, since there's almost surely going to be a single instance in the whole application performing all the evaluations. I think this is the same reason why OpenFeature server-side SDKs don't provide a way to mutate the context, and instead require that each flag evaluation have an explicit context. I agree with this approach.

state.go

client.go

state.go

gagantrivedi · 2025-03-26T03:19:21Z

client.go

 	// Initialise analytics processor
-	if c.config.enableAnalytics {
-		c.analyticsProcessor = NewAnalyticsProcessor(c.ctxAnalytics, c.client, c.config.baseURL, nil, c.log)
+	if c.ctxAnalytics != nil {


Can we add the variable back? I'd prefer not to do if else on context

Can we also a test here to make sure if analytics processor is enabled, it is initialised properly?

gagantrivedi · 2025-03-26T03:26:04Z

client.go

-	if ok {
-		envCtx := ec.Environment
-		if envCtx != nil {
-			req.SetHeader(EnvironmentKeyHeader, envCtx.APIKey)


@khvn26 do you remember why we did this? As far I can remember, it was because a customer wanted it?

I think it was just general support for overriding the evaluation context

gagantrivedi · 2025-03-26T03:28:10Z

client.go

@@ -205,92 +210,100 @@ func (c *Client) BulkIdentify(ctx context.Context, batch []*IdentityTraits) erro
 		SetBody(&body).
 		SetContext(ctx).
 		ForceContentType("application/json").
-		Post(c.config.baseURL + "bulk-identities/")
-	if resp.StatusCode() == 404 {


Why did we get rid of this?

I don't see any value in handling this error. A 404 could be returned by an intermediate proxy so the suggestion to make sure Edge is being used is not always correct.

The better solution here would be to implement the /bulk-identities endpoint on the Core API, but have it return 404 and a relevant error message.

realtime.go

gagantrivedi · 2025-03-26T04:05:52Z

offline_handler.go

@@ -7,34 +7,26 @@ import (
 	"github.com/Flagsmith/flagsmith-go-client/v4/flagengine/environments"
 )

-type OfflineHandler interface {
-	GetEnvironment() *environments.EnvironmentModel
+type Environment interface {


This is just confusing—now we have an interface with the same name as the method. I don't really think we're gaining anything by breaking the old interface?

The name "Handler" is misleading. This type was not being used as a handler because it's not called in response to any event; the offline environment was only being initialised when the client is constructed and never again. For example, http.Handler is named correctly because it's called in response to a request, but this was not the case for OfflineHandler.

I'll rename the method in this interface back to GetEnvironment

gagantrivedi · 2025-03-26T04:51:03Z

state.go

+	return nil
+}
+
+func (cs *environmentState) SetEnvironment(env *environments.EnvironmentModel) {


Do we have any tests for functions defined in the file?

Not directly, I'll add them later

gagantrivedi · 2025-03-26T04:54:20Z

client.go

-		}
-		return f
-	}
-	if resp.StatusCode() != 200 {


Why did we get rid of this check? Anything other than 200 should be error for us?

This wasn't removed, just changed to !resp.IsSuccess. The diff is confusing because this method was moved in the file. The check is still there: https://github.com/Flagsmith/flagsmith-go-client/pull/151/files/7dcb4a00171dc3666b2a770a7e573730f9fe9a80#diff-4b667feae66c9d46b21b9ecc19e8958cf4472d162ce0a47ac3e8386af8bbd8cfR177

gagantrivedi · 2025-03-26T04:57:55Z

client.go

-
-	analyticsProcessor *AnalyticsProcessor
+	baseURL            string
+	timeout            time.Duration


Why did we move timeout and proxy out of options and set them directly on the client? Are we planning to use them somewhere else?

Just convenience, I didn't feel it was worth having a container type to hold the different settings so I've added them directly to the client. I can revert if you feel strongly about it but I don't think it makes a difference either way.

gagantrivedi · 2025-03-26T05:03:13Z

client_test.go


-	flags, err := client.GetFlags(ctx, nil)
+	flags, err := client.GetEnvironmentFlags(context.Background())


TestGetFlags is testing GetEnvironmentFlags here?

rolodato added 7 commits March 21, 2025 21:13

Use structs instead of pointers for evaluation context

57b3128

v5

78027a2

Revert WithDefaultFlagHandler

0fbb714

Revert change to NewTransientEvaluationContext

1e23a8c

Remove atomics + more improvements

dd7e12a

flush

f7bb917

use logger group

e79acfd

rolodato force-pushed the feat/event-channel branch from a050a54 to e79acfd Compare March 22, 2025 21:56

rolodato added 7 commits March 22, 2025 19:09

store environment + overrides together without atomics

5c61285

restore TraitModel

b65322a

default logger

7de1240

MustNewClient

0a6c698

refactor

527d1b7

undeprecate

e101db8

log

bcc3e1c

rolodato force-pushed the feat/event-channel branch from 33aefe0 to bcc3e1c Compare March 23, 2025 00:47

more logs

cf672ff

rolodato force-pushed the feat/event-channel branch from 6394d25 to cf672ff Compare March 23, 2025 01:55

rolodato added 2 commits March 22, 2025 23:05

logs

0521abe

debug logs in tests

1966d3c

rolodato marked this pull request as ready for review March 23, 2025 02:13

rolodato changed the title ~~feat! v5~~ feat!: v5 Mar 23, 2025

rolodato force-pushed the feat/event-channel branch from b38875c to fb512ba Compare March 23, 2025 02:55

more logs + pass EnvironmentContext structs and not pointers

d0e37a1

rolodato force-pushed the feat/event-channel branch from fb512ba to d0e37a1 Compare March 23, 2025 03:02

rolodato added 5 commits March 23, 2025 00:50

remove unused

813dd1b

ci disable cache

ba818e3

disable actions/setup-go cache

c79c51e

remove environment from EC

f40dfb4

lint

f3a98df

rolodato added 11 commits March 23, 2025 02:13

misc

8edb88d

simpler offline mode

28bdcd9

simplify error types

bf31c3e

refactor

c8a6d18

Docs

1e79009

revert to pointer EnvironmentModel

21ec13f

goimports

eae8a45

remove codegen

4704300

goimports

f3f75d0

fix GetIdentityOverride data race

02e81f9

restore ci changes

588a5fb

khvn26 approved these changes Mar 24, 2025

View reviewed changes

state.go Show resolved Hide resolved

rolodato mentioned this pull request Mar 25, 2025

feat!: Poll and use realtime at the same time #153

Draft

gagantrivedi requested changes Mar 25, 2025

View reviewed changes

rolodato added 2 commits March 25, 2025 09:02

re-add analytics

d5bfe23

feedback

7d61b9d

gagantrivedi reviewed Mar 25, 2025

View reviewed changes

state.go Outdated Show resolved Hide resolved

more feedback

7dcb4a0

rolodato requested a review from gagantrivedi March 25, 2025 13:57

gagantrivedi requested changes Mar 26, 2025

View reviewed changes

feedback

7f3bd06

rolodato force-pushed the feat/event-channel branch from c9e8ea2 to 7f3bd06 Compare March 26, 2025 12:13

rolodato marked this pull request as draft March 27, 2025 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: v5 #151

feat!: v5 #151

rolodato commented Mar 22, 2025 •

edited

Loading

khvn26 left a comment

gagantrivedi left a comment

gagantrivedi Mar 25, 2025

rolodato Mar 25, 2025

gagantrivedi Mar 25, 2025

rolodato Mar 25, 2025

gagantrivedi Mar 26, 2025

gagantrivedi Mar 26, 2025

gagantrivedi Mar 26, 2025

khvn26 Mar 26, 2025

gagantrivedi Mar 26, 2025

rolodato Mar 26, 2025

gagantrivedi Mar 26, 2025

rolodato Mar 26, 2025

gagantrivedi Mar 26, 2025

rolodato Mar 26, 2025

gagantrivedi Mar 26, 2025

rolodato Mar 26, 2025

gagantrivedi Mar 26, 2025

rolodato Mar 26, 2025

gagantrivedi Mar 26, 2025

rolodato Mar 26, 2025


		flags, err := client.GetFlags(ctx, nil)
		flags, err := client.GetEnvironmentFlags(context.Background())

feat!: v5 #151

Are you sure you want to change the base?

feat!: v5 #151

Conversation

rolodato commented Mar 22, 2025 • edited Loading

Breaking changes

Removed support for transient traits and transient evaluations

Simplified evaluation context types

Removed WithOfflineHandler in favour of WithOfflineEnvironment

Simplified error types

NewClient no longer panics

Removed custom logger interface

New features

Bug fixes

Other changes

khvn26 left a comment

Choose a reason for hiding this comment

gagantrivedi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolodato commented Mar 22, 2025 •

edited

Loading

Removed `WithOfflineHandler` in favour of `WithOfflineEnvironment`