feat: introduce features/settings client

[lvrach]

Description

This PR is meant to introduce functionality that will inform the control plane about the features of rudder-server. Control-plane will make decisions based on what rudder-server supports or not.

More about this can be read in notion.

Registry

I was puzzled about the best way to capture features of multiple components as they are added in rudder-server, global vs injected

One approach is to DI the functionality in every component, which seemed overkill for two reasons:

• This information is relatively static; it will be modified only when new features are added and won't change based on logic.
• The amount of code needed to change for DI to be adequately introduced would be extensive.

I end up using a global registry approach, for now, similar to what we do for configs, stats and logs. This should be revised in the future, especially for logs and stats DI will be handy for testing.

Client

I wanted to decouple the client for Registry. Both for testing and extensibility. Registry in a simple in-memory store. While client just takes the Registry and sends the payload to the control plane.

Registry assumes all features will be added before a client call is made. We might want to revisit the decision if a feature should register based on configuration (feature flags). However, a system that waits until all registrations have occurred would require more complexity, so it is deferred until needed.

With the current implementations, features need to be registered in an init method.

With this approach, I wanted to avoid:

1. Sending a message every time we register a feature.
2. Use a debounce logic, where we wait for an amount of time to gather and batch multiple features.
3. Use pre-registration and a lock/channel mechanism waiting for everything to be registered before we send it.

Some of the rejected approaches above might need to revisit as our requirements, code pollution, and usage patterns become more advanced.

Identity

I have also introduced an abstraction called identity. I wanted this abstraction to be under control-plane-specific libraries and address the two different ways a rudder-server can be identified by control-plane workspace and namespace.

The differences between workspace and namespace are minor, with a slightly different endpoint and a different authorisation mechanism. Instead of having two different clients for workspace and namespace, I've delegated/abstracted the differences to the identity interface.

I also want to introduce identity in backend config and Oauth client, and remove the AccessToken method, which doesn't encapsulate the auth mechanism and the type of identity we are using. For now, I've marked as deprecated as replacing AccessToken would be beyond the scope of this task.

Removed legacy multitenant code

Backend config had an additional mechanism two support multitenant without namespaces. This mechanism didn't support /settings as it was deprecated. I didn't want to perplex things anymore with this legacy code, so I've removed it.

Notion Ticket

https://www.notion.so/rudderstacks/Reporting-features-to-control-plane-dd4976cb288544cf979f1d1e85913c1b

Security

• The code changed/added as part of this pull request won't create any security issues with how the software is being used.

[codecov]

Codecov Report

Base: 38.62% // Head: 38.56% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (781ac09) compared to base (d43705e).
Patch coverage: 63.24% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2377      +/-   ##
==========================================
- Coverage   38.62%   38.56%   -0.06%     
==========================================
  Files         166      168       +2     
  Lines       36696    36740      +44     
==========================================
- Hits        14173    14169       -4     
- Misses      21608    21660      +52     
+ Partials      915      911       -4     

Impacted Files	Coverage Δ
config/backend-config/mock_workspaceconfig.go	9.86% <0.00%> (-1.17%)	⬇️
config/backend-config/noop.go	0.00% <0.00%> (ø)
warehouse/warehouse.go	3.10% <0.00%> (-0.03%)	⬇️
services/controlplane/features/client.go	83.72% <83.72%> (ø)
config/backend-config/backend-config.go	67.46% <100.00%> (-1.85%)	⬇️
config/backend-config/namespace_config.go	79.56% <100.00%> (+0.93%)	⬆️
config/backend-config/single_workspace.go	64.16% <100.00%> (+1.55%)	⬆️
main.go	72.85% <100.00%> (+1.69%)	⬆️
utils/httputil/client.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[lvrach]

Waiting for config to send is a workaround since we don't have the workspaceID, until config is fetched.

There is a plan to remove this need in the v2 API.

[fracasula]

Suggested change

	defer resp.Body.Close()
	b, err := io.ReadAll(resp.Body)
	b, err := io.ReadAll(resp.Body)
	_ = resp.Body.Close()

Optional of course but I wanted to share to gather your thoughts given that in this case it seems very easy to avoid deferring.

package features

import (
	"testing"
)

func doNoDefer(t *int) {
	func() {
		*t++
	}()
}

func doDefer(t *int) {
	defer func() {
		*t++
	}()
}

func BenchmarkDefer(b *testing.B) {
	b.Run("yes", func(b *testing.B) {
		t := 0
		for i := 0; i < b.N; i++ {
			doDefer(&t)
		}
	})

	b.Run("no", func(b *testing.B) {
		t := 0
		for i := 0; i < b.N; i++ {
			doNoDefer(&t)
		}
	})
}

With go 1.19 it gives me:

goos: linux
goarch: amd64
pkg: github.com/rudderlabs/rudder-server/services/controlplane/features
cpu: 12th Gen Intel(R) Core(TM) i9-12900HK
BenchmarkDefer
BenchmarkDefer/yes
BenchmarkDefer/yes-20         	588573512	         2.031 ns/op
BenchmarkDefer/no
BenchmarkDefer/no-20          	1000000000	         0.2673 ns/op
PASS

Perhaps I'm not measuring it appropriately but it seems like some overhead is there 🤔

(I'm not saying do not use defer, just think twice if it's not really needed. If you have multiple if/else with returns then paying the overhead makes sense for the sake of keeping the complexity under control, in any case again totally optional).

[lvrach]

Interesting, I would not expect this to be that big.

However, the performance overhead of using defer in this case should be minor. Indeed, in this case, it can be easily avoided, but I like it as a consistent pattern across the code. I expect a defer close as soon as a successful request has returned.

For me, the readability/predictability ways the performance when IO is involved.

[fracasula]

Yes I agree with you. I'd say that having the defer makes it easier to follow if you're coming from a Go background.

It is a micro optimization on a piece of code that is not really giving us problems. Like yourself I think that readability should take precedence for the sake of code maintenability.

I wanted to share this anyway because sometimes I avoid using defer myself for the performance penalty (when the code is quite simple and doesn't have returns). Plus sharing is caring so 😄

[fracasula]

LGTM 👍
I've added a comment to fix a typo and another one as a reminder, I see that you reacted with 👍 but forgot to add the error context.

[atzoum]

how and when is this going to be populated?

[lvrach]

They are going to be populated manually for now.

When a new feature that the control plane needs to be aware of is developed, it will be added here.

Following your comment here #2377 (comment), I found this approach to be the most straightforward and cleanest for now.

The dynamic enabled/disable of features based on config is not supported yet, but it can be easily implemented if required by something like this:

func ServerComponentFromConfig(conf config.Config) Component {

  comp := Component{
	Name: "server",
	Features: []string{},
  }
  if config.IsSet("feature.enabled") {
  	comp.Features = append(comp.Features, "features")
  }
  
  return comp
}

[atzoum]

nobody seems to be using these

[lvrach]

I will remove the file and introduce it in another PR.