[flagd] follow-up refactoring

[thisthat]

Tasks

Preview Give feedback

1. Improve retry-logic for gRPC stream (ctx)
2. Abstract gPRC into a generic service (ctx)
3. Improve initialize(EvaluationContext) strategy (ctx)
4. Keep the latest known cache status in case of error (ctx)
5. Use always OTel interceptor (ctx)

[toddbaert]

See my comment here, I'm not sure we can really do much about the 4th point.

The rest looks good.

[Kavindu-Dodan]

I will work on this :)

[Kavindu-Dodan]

@thisthat except for this, we have completed all other tasks. And this last one is not necessary for the current operation of flagd. Further, in-process provider proposed through #411 will provider more options to use flagd.

I am voting to close this as complete

[thisthat]

I think also the last point (always use OTel) is not yet done.
In the code (link), we instrument the gRPC streams only if we pass an SDK instance instead of acting as a library and get the default tracer all the time. OTel docs: https://opentelemetry.io/docs/concepts/instrumentation/libraries/

[thiyagu06]

I think also the last point (always use OTel) is not yet done. In the code (link), we instrument the gRPC streams only if we pass an SDK instance instead of acting as a library and get the default tracer all the time. OTel docs: https://opentelemetry.io/docs/concepts/instrumentation/libraries/

We already discussed about this in #286 (comment). are we changing from this stance and planning to use GlobalOpenTelemetry ?

[Kavindu-Dodan]

@thisthat could you please explain your suggestion in detail? I cannot fully understand the requirements you are proposing.

Besides @thiyagu06 has a good point about using global telemetry.

[thisthat]

Sorry to overlook this ticket for so long @thiyagu06 and @Kavindu-Dodan 🤦
The problem is with the following lines:

java-sdk-contrib/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/grpc/strategy/ResolveFactory.java

Lines 15 to 18 in 2f2ce59

	if (options.getOpenTelemetry() != null) {
	return new TracedResolving(options.getOpenTelemetry());
	}
	return new SimpleResolving();

If we don't pass an OTel instance, gRPC will never be instrumented. This is a problem when you work with Agent auto-instrumentation that provides an SDK implementation at runtime.

Since auto-instrumentation for Java is quite powerful (see OTel java-instrumentation or other Observability vendors), I would suggest changing the lines before. If the App developer didn't provide an OpenTelemetry instance, we default to using theGlobalOpenTelemetry one. This way, also with auto-instrumentation we can attach to the gRPC spans the FF evaluation details.

[Kavindu-Dodan]

I checked the concerned usage and I have the following suggestion,

• We should still allow traces-free simple resolving
• Give an option to use global OTel
• Continue to support OTel SDK injection through builder

This fulfils all the requirements.

[Kavindu-Dodan]

@thisthat please check this PR with the fix - #505

This is the best approach I could think of

[thisthat]

We should still allow traces-free simple resolving

Wouldn't this be achieved through the no-op implementation of the Otel API? 🤔

[Kavindu-Dodan]

We should still allow traces-free simple resolving

Wouldn't this be achieved through the no-op implementation of the Otel API? 🤔

We could, but that means we have a few extra calls backed by no-op implementation. And if someone wants to avoid those calls, traces-free resolving helps.