feat: otel interceptor for flagd go-sdk #176
Conversation
Kavindu-Dodan
changed the title
| @@ -1,64 +0,0 @@ | |||
| // Code generated by MockGen. DO NOT EDIT. | |||
There was a problem hiding this comment.
removed as we do not use this anymore
| mockgen: | ||
| mockgen -source=pkg/service/iservice.go -destination=internal/mock/service_mock_test.go -package=mock | ||
| mockgen -source=pkg/service/client.go -destination=pkg/service/client_mock_test.go -package=service_test | ||
| mockgen -source=pkg/service/iservice.go -destination=internal/mock/service_mock.go -package=mock |
There was a problem hiding this comment.
This is a correction which we missed to update
Kavindu-Dodan
requested review from
beeme1mr,
james-milligan,
thisthat and
toddbaert
Kavindu-Dodan
force-pushed
the
feat/otel-interceptor
branch
from
d7b8a14 to
0bd3d48
Compare
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Kavindu-Dodan
force-pushed
the
feat/otel-interceptor
branch
from
0bd3d48 to
71d7785
Compare
| flagd.WithLRUCache(1000) // enables LRU caching (see configuring caching section) | ||
| flagd.WithBasicInMemoryCache() // enables basic in memory cache (see configuring caching section) | ||
| flagd.WithLogger(logger) // sets a custom logger (see logging section) | ||
| flagd.WithOtelInterceptor(bool) // enable or disable OpenTelemetry interceptor for flagd communication |
There was a problem hiding this comment.
What would the impact be of having this enabled by default?
There was a problem hiding this comment.
Interceptor uses no-op implementations and yields nothing
The idea here was to allow sdk users to manually opt-in for telemetry interception. Because this is only needed if the sdk user (i.e- application developer) has a proper telemetry configuration for the application. Setup includes exporter configuration (ex:- console, otel exporter ...) and related services up and running (ex:- otel collector).
So for simple usage as well as to exclude the performance impact of including no-op interceptors, it's best to make this an opt-in configuration.
| "google.golang.org/protobuf/types/known/structpb" | ||
| ) | ||
|
|
||
| type ServiceConfiguration struct { |
There was a problem hiding this comment.
moved to client.go where this is properly used
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| if c.client != nil { | ||
| return c.client | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we can do the nil check first, and only lock if c.client is nil while we create it. That way, we only bother doing any locking if the c.client has not been created. Setting it is atomic, I think... so once it's been set and we unlock I guess we can never worry about locking again.
As is, this will lock with every access, which might not be needed. What do you think?
There was a problem hiding this comment.
After a second look at the code, I think the lock is an overkill. I will try to refactor the code to be bit nicer
There was a problem hiding this comment.
Refactored - no more locks and null checks. We have NewClient to generate the client [1]
Signed-off-by: Kavindu Dodanduwa <[email protected]>
4 participants
This PR
Fulfill the sdk-side implementation of issue open-feature/flagd#620
Introduce option to enable disable otel interceptor for flagd connection from flagd go-sdk
SDK configuration -
openfeature.SetProvider(flagd.NewProvider(flagd.WithOtelInterceptor(true)))This option combined with open telemetry configurations at sdk usage component (ex:- application), allows distributed tracing for flagd
How to test
Write a simple code with otel traces and propagator, and use flagd with the option
flagd.WithOtelInterceptor(true)Related PR - open-feature/flagd#624