refactor: use attribute.KeyValue instead of map
#418
Conversation
Signed-off-by: Sahid Velji <[email protected]>
| // FlagEvaluationKey is the name of the feature flag evaluation event. | ||
| const FlagEvaluationKey string = "feature_flag.evaluation" | ||
| // EventName is the name of the feature flag evaluation event. | ||
| const EventName string = "feature_flag.evaluation" |
There was a problem hiding this comment.
this is what you & Michael were asking when it would be added to semconv in #407 correct?
There was a problem hiding this comment.
Yes, it doesn't currently exist in the latest semconv 1.34.0, which is why this const is needed for now
https://pkg.go.dev/go.opentelemetry.io/[email protected]/semconv/v1.34.0#section-readme
There was a problem hiding this comment.
I wanted to put here that I think returning the []attribute.KeyValue seems to me like the better move. I think getting that inherent telemetry integration makes the friction between using OpenFeature & OpenTelemetry together less, and like you said it isn’t too much of a stretch to assume that both are being used when someone uses OpenFeatue.
and to make sure I understand how users will get the a specific value they will do like this correct?
attrSet := attribute.NewSet(attrs...)
provider, found := attrSet.Value(semconv.FeatureFlagProviderNameKey)
if found {
// use provider
}Because I think with good docs that is pretty reasonable. In my initial look and understanding I am liking this approach it looks like it will enforce the type safety we want, be cleaner, minimize issues with using the Otel (spans and such) and keep order of attributes which will be nice.
Yes that's right. |
| attributes := map[string]any{ | ||
| FlagKey: hookContext.FlagKey(), | ||
| ProviderNameKey: hookContext.ProviderMetadata().Name, | ||
| func EventAttributes(hookContext openfeature.HookContext, details openfeature.InterfaceEvaluationDetails) []attribute.KeyValue { |
There was a problem hiding this comment.
Since this is a new function, should we keep the old one with a deprecation warning? I don't want technical debt to pile up, but this seems like an easy way to avoid abruptly adding a breaking change.
There was a problem hiding this comment.
We could branch our v1 now and this could be a breaking change, but I think we should consider this.
If we want to implement this in contribs instead, to avoid the OTel dep (which would be my preference) I'd recommend we just cut the v1 maintenance branch now, and then completely remove this function here and implement it in contribs.
WDYT @beeme1mr @sahidvelji ?
| require ( | ||
| github.com/cucumber/godog v0.15.0 | ||
| github.com/go-logr/logr v1.4.3 | ||
| go.opentelemetry.io/otel v1.37.0 |
There was a problem hiding this comment.
Generally we like to keep our SDK free of non-test dependencies. Of course, we can make exceptions for some cases, and OTel could be such an exception, but I think I'd advocate for implementing this in the contrib as you alluded to in slack.
I think if we are going to implement it in the SDK directly, we should avoid this dependency. The only SDK that includes OTel bits directly is .NET, but that's a bit of a different case since recent .NET runtimes actually include OTel.
There was a problem hiding this comment.
@beeme1mr @sahidvelji my main question is, how realistic is it for an org to reject the use of OpenFeature because of this dep? Is OTel "controversial" for some orgs? Might a big org that offers telemetry products, which would otherwise adopt OpenFeature, decide not to just because of this dep?
There was a problem hiding this comment.
So Go dependency "tree shaking" happens at the package level. So unless a user imports the telemetry package, they will not get the OTel dependency. But again, given that the OTel hooks live in contrib instead of the SDK, I think it might make sense for this package to also live in contrib instead.
There was a problem hiding this comment.
I think this should be fine given how it's used.
|
I'm in favour of adding a telemetry package in contrib and just deprecating the telemetry package in this repo.
There are a couple more things I want to address before doing this |
|
@beeme1mr @toddbaert Just following up here. Are we OK with deprecating the telemetry package here and creating it in contrib? I think that makes more sense given that the OTel hooks package also lives in contrib. |
This PR
I still need to fix the tests and also add more doc comments, but I wanted to get alignment from others before going ahead with those changes.
Related Issues
close #393
close #407
Notes
Follow-up Tasks
How to test