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. |
@beeme1mr @toddbaert @aepfli @erka @bbland1 I'd like to pick up this discussion again. Please comment. |
|
@sahidvelji rereading and refreshing myself, I would say that I agree with what felt like the previous consensus to deprecate the telemetry package here and have it in the contrib. It stays consistent with what other SDKs and with the Otel hook package there it kind of keeps those concepts in similar areas to look for info on when contributing to the dev or using it in their projects. |
|
I support your proposal @sahidvelji. Since |
|
Whatever we decide here, we should be consistent across all the |
|
Hey @sahidvelji, we added the telemetry method to the SDKs because I thought it would be generally useful (even outside of OTel) and would be dependency-free. If we bring in the OTel semcon dependency, that does change the equation slightly. Arguably, we don't need the dependency because the semcon shouldn't change much. I'm also one of the approvers in OTel, so hopefully changes won't go unnoticed. Having said that, I don't really have an issue with moving the logic entirely to a hook. Nothing in the OpenFeature spec requires the telemetry function to live in the SDK. The opposite extreme is .NET, where OTel is built into the runtime, and we're considering adding OTel support natively in the SDK itself. I believe end users won't care where this logic works as long as there's an easy way to capture spec-compliant and consistent telemetry signals. I was working with @lukas-reining on the JavaScript OTel hook last week. Pulling him in to see if he has a strong opinion. If not, I'll defer to @sahidvelji and @erka to decide what makes the most sense in the Go SDK. |
5 participants
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