Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Feature Flag Service in favor of OpenFeature + flagd #1388

Merged
merged 25 commits into from
Mar 7, 2024

Conversation

austinlparker
Copy link
Member

@austinlparker austinlparker commented Feb 18, 2024

Changes

This PR removes the Erlang featureflag service and its dependencies in favor of OpenFeature ecosystem projects.

Rationale

Feature Flag Service is a bit of a heavy dependency for the project to maintain. We don't have any regular Erlang contributors in maintainer/approver ranks, which makes it difficult to evaluate changes. There's been a fairly long-standing issue with cross-platform builds in QEMU and various Erlang components which has kept the service on outdated versions of OTP. We've had several feature requests for more flexibility around the feature flags that require setting via API, and the lack of a dedicated client for the feature flag service itself makes it somewhat challenging for contributors to add new things (as they have to write handlers for each application).

OpenFeature is a CNCF incubating project that's under active development and provides several things:

  • Client libraries for most languages
  • Lightweight flag daemon (flagd)
  • Kubernetes integration
  • Native OpenTelemetry support

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@austinlparker austinlparker marked this pull request as ready for review February 19, 2024 00:40
@austinlparker austinlparker requested a review from a team February 19, 2024 00:40
@julianocosta89
Copy link
Member

Wow!
This is a huge change but I'd hold it for a bit.
I wouldn't replace the FeatureFlagService without having another Erlang/Elixir service on the demo.

@austinlparker
Copy link
Member Author

Wow! This is a huge change but I'd hold it for a bit. I wouldn't replace the FeatureFlagService without having another Erlang/Elixir service on the demo.

See #1343 (comment) -- I think we'll need to get another Erlang service added in as a stub off Kafka somehow. I don't necessarily think it should block this PR, though.

@julianocosta89
Copy link
Member

Wow! This is a huge change but I'd hold it for a bit. I wouldn't replace the FeatureFlagService without having another Erlang/Elixir service on the demo.

See #1343 (comment) -- I think we'll need to get another Erlang service added in as a stub off Kafka somehow. I don't necessarily think it should block this PR, though.

So you would drop Erlang/Elixir from the demo till we get a new service rewritten?

@julianocosta89
Copy link
Member

I vote against it.

@austinlparker
Copy link
Member Author

We can discuss it at the next SIG meeting. I don't anticipate releasing the demo without an Erlang service, but we also can't arbitrarily hold off on integrating new changes because of implicit dependencies.

@tsloughter
Copy link
Member

Wish I'd known about this I'd have done what I could to make it not necessary, I wasn't aware it was being considered. Does Erlang/Elixir now get no demo app?

@austinlparker
Copy link
Member Author

Wish I'd known about this I'd have done what I could to make it not necessary, I wasn't aware it was being considered. Does Erlang/Elixir now get no demo app?

No, we'll still have an Erlang demo app, but probably a smaller one that's less of a maintenance burden?

@tsloughter
Copy link
Member

So I can try to help, what burdern does it ease to be another service? You mention the build pipeline, how would that be different for product service or the new kafka service?

@austinlparker
Copy link
Member Author

@tsloughter moving this convo over to #1343

@beeme1mr
Copy link
Contributor

Hey @austinlparker, I'm one of the maintainers of OpenFeature. Thanks for getting this started! Please let me know if you have any questions.

@agardnerIT
Copy link

I'd have done what I could to make it not necessary

For the future, calls to the OpenFeature SDK can specify a default value so even if the backend is unavailable, you'll still get a working app.

@austinlparker
Copy link
Member Author

I'd have done what I could to make it not necessary

For the future, calls to the OpenFeature SDK can specify a default value so even if the backend is unavailable, you'll still get a working app.

Feel free to check my work but I'm pretty sure that's what I've done here.

@austinlparker
Copy link
Member Author

Hey @austinlparker, I'm one of the maintainers of OpenFeature. Thanks for getting this started! Please let me know if you have any questions.

Rad! Could you look into getting the python flagd provider released? :)

@beeme1mr
Copy link
Contributor

Rad! Could you look into getting the python flagd provider released? :)

@thisthat has opened a PR. Hopefully, we can get publishing working tomorrow.

open-feature/python-sdk-contrib#38

@austinlparker
Copy link
Member Author

Perfect!

@tsloughter
Copy link
Member

@basti1302 nice, thanks, wish it would have worked out. I would fight this if it wasn't for the QEMU issue on M1+ -- and now I think at this point it is too far along to be worth finding out if the issue is resolved in OTP-27-rc.

Copy link
Contributor

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from an OpenFeature perspective. Once this has been merged, I could help set up the OpenTelemetry hooks, which provide span events and metrics based on the OTel semantic convention. We could also add client-side feature flags if there are any use cases you would like to support.

Thanks for your effort on this. @dyladan and I have a session at KubeCon talking about OpenTelemetry and OpenFeature. It would be amazing to use the OTel demo app for our examples.

src/flagd/demo.flagd.json Outdated Show resolved Hide resolved
@mviitane
Copy link
Member

When running this branch, I constantly get this error:
frontend | Error: 12 UNIMPLEMENTED: Method not found: oteldemo.AdService/GetAds
Anyone else see this?

@julianocosta89
Copy link
Member

@mviitane yes!

Error: 12 UNIMPLEMENTED: Method not found: oteldemo.AdService/GetAds
    at callErrorFromStatus (/app/node_modules/@grpc/grpc-js/build/src/call.js:31:19)
    at Object.onReceiveStatus (/app/node_modules/@grpc/grpc-js/build/src/client.js:192:76)
    at Object.onReceiveStatus (/app/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:360:141)
    at Object.onReceiveStatus (/app/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:323:181)
    at /app/node_modules/@grpc/grpc-js/build/src/resolving-call.js:99:78
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)
for call at
    at ServiceClientImpl.makeUnaryRequest (/app/node_modules/@grpc/grpc-js/build/src/client.js:160:32)
    at ServiceClientImpl.<anonymous> (/app/node_modules/@grpc/grpc-js/build/src/make-client.js:105:19)
    at /app/node_modules/@opentelemetry/instrumentation-grpc/build/src/grpc-js/clientUtils.js:131:31
    at /app/node_modules/@opentelemetry/instrumentation-grpc/build/src/grpc-js/index.js:233:209
    at AsyncLocalStorage.run (node:async_hooks:338:14)
    at AsyncLocalStorageContextManager.with (/app/node_modules/@opentelemetry/context-async-hooks/build/src/AsyncLocalStorageContextManager.js:33:40)
    at ContextAPI.with (/app/node_modules/@opentelemetry/api/build/src/api/context.js:60:46)
    at ServiceClientImpl.clientMethodTrace [as getAds] (/app/node_modules/@opentelemetry/instrumentation-grpc/build/src/grpc-js/index.js:233:42)
    at /app/.next/server/pages/api/data.js:63:58
    at new Promise (<anonymous>) {
  code: 12,
  details: 'Method not found: oteldemo.AdService/GetAds',
  metadata: Metadata {
    internalRepr: Map(1) { 'content-type' => [Array] },
    options: {}
  }
}

@austinlparker was that working on the initial PR?

@julianocosta89
Copy link
Member

Ads are not being loaded at all:
image

@austinlparker
Copy link
Member Author

Huh. I thought it was?

@austinlparker
Copy link
Member Author

Hm, does seem like it's not working. The ad service is running fine though.

@austinlparker
Copy link
Member Author

Should be working now, my bad.

@austinlparker
Copy link
Member Author

@beeme1mr looks like the python otel hook isn't published yet?

@austinlparker
Copy link
Member Author

See #1436 for notes on how we will integrate with k8s

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it, and everything seems to be working fine.
Let's ship it! 🚀

@austinlparker austinlparker merged commit ce4ad93 into open-telemetry:main Mar 7, 2024
26 checks passed
@beeme1mr
Copy link
Contributor

beeme1mr commented Mar 7, 2024

@beeme1mr looks like the python otel hook isn't published yet?

We'll publish the hook and include it in a follow up PR.

@federicobond
Copy link
Member

federicobond commented Mar 14, 2024

@austinlparker the Python OTel hook is now released! Note that you need to update the import paths for the released version

https://pypi.org/project/openfeature-hooks-opentelemetry/

@julianocosta89
Copy link
Member

@federicobond I've tried adding it, but it seems there is an issue with the hook.
I've opened an issue here: open-feature/python-sdk-contrib#67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants