open-feature
refactor: use `attribute.KeyValue` instead of map
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
to make sure I understand how users will get the a specific value they will do like this correct?
Yes that's right.
I'm in favour of adding a telemetry package in contrib and just deprecating the telemetry package in this repo.
I'd recommend we just cut the v1 maintenance branch now
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 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 semconv has already provides the majority of necessary attributes, I don’t think the telemetry package adds significant value to go-sdk. Even the otel hook doesn’t rely on it.
Whatever we decide here, we should be consistent across all the sdk repos. @beeme1mr Let me know if there is a better place to discuss proposals that affect all SDKs.
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.
Any updates here? @beeme1mr @lukas-reining
I still don't have a strong opinion on where this logic should live, as long as the functionality is spec-compliant. I'm hoping either today or tomorrow to expand on the observability appendix in the OpenFeature docs to capture the requirements of the telemetry hook. You can see the PR @lukas-reining wrote here. The docs I'm planning on writing will be based on the discussions we had while building that.