feat: Expose set sdk metadata #166
Conversation
vahidlazio
requested review from
fabriziodemaria,
nicklasl and
nickybondarenko
as code owners
vahidlazio
force-pushed
the
expose-set-sdk-id
branch
from
8a6f0a0 to
5f9d105
Compare
| fun setSdk(sdk: SdkMetadata) { | ||
| (flagResolver as RemoteFlagResolver).setSdk(sdk) | ||
| (eventSenderEngine as EventSenderEngineImpl).setSdk(sdk) | ||
| (flagApplierClient as FlagApplierClientImpl).setSdk(sdk) | ||
| } |
There was a problem hiding this comment.
Is it really not possible to only support passing it in the create() function?
Having it mutable isn't really something we want to support, right?
There was a problem hiding this comment.
yes but then what would be the default? if someone wants to use confidence in conjunction with open feature, do we really want to force them to pass the correct Id or additional parameter when creating confidence? or we want to do it automatically?
another way is to pass the apiKey and other confidence parameters to the provider creation function and then provider to create the confidence.
There was a problem hiding this comment.
we can make it immutable but take into account that users of open feature, in order to have the correct id, should pass SDK_ID_KOTLIN_PROVIDER, and then my worry is, it will be easy for them to just not do it and our telemetry will be incorrect.
There was a problem hiding this comment.
Hmm... I didn't think about the OpenFeature provider case but purely for Flutter...
Can we explore different naming for create functions depending on the use case?
Or I wonder if we can pivot on the creation of the OF provider somehow to have it create the Confidence-instance behind the scenes or something? -- a bit strange since we likely want to also keep the reference to Confidence.
There was a problem hiding this comment.
"a bit strange since we likely want to also keep the reference to Confidence."
exactly!
for flutter, I already added in this PR the sdk: SdkMetadata parameter which can be passed in create function. setSdk is/will be called only within the provider context.
…or creating confidence with the correct sdk id
| initialContext: Map<String, ConfidenceValue> = mapOf(), | ||
| region: ConfidenceRegion = ConfidenceRegion.GLOBAL, | ||
| dispatcher: CoroutineDispatcher = Dispatchers.IO | ||
| ): Confidence { |
There was a problem hiding this comment.
I am wondering if we could introduce a new interface ConfidenceForOpenFeature or something, and make the create function below accept that, as a way to force users to use this createConfidence
There was a problem hiding this comment.
in this case ConfidenceForOpenFeature should inherit both from event sender and from Contextual
There was a problem hiding this comment.
and all other functions are from confidence object itself, we don't have a confidence interface currently.
There was a problem hiding this comment.
Perhaps worth exploring, but I am also not strongly convinced these extra interfaces will actually help the user
There was a problem hiding this comment.
do you agree taking this in another PR and moving forward with this PR?
This reverts commit 10548d9.
No description provided.