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

[paymentservice] Add paymentServiceFailure & paymentServiceUnreachable featureflags #1383

Closed
wants to merge 0 commits into from

Conversation

EislM0203
Copy link
Contributor

@EislM0203 EislM0203 commented Feb 15, 2024

Changes

Add the featureflags paymentServiceFailure & paymentServiceUnreachable.

  • paymentServiceFailure: fails calls to the charge method of the paymentservice
  • paymentServiceUnreachable: fails calls to paymentservice itself

Added grpc-ff-client.js for communication with the featureflagservice.

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

Copy link

linux-foundation-easycla bot commented Feb 15, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@EislM0203 EislM0203 changed the title [paymentservice] add paymentServiceFailure & paymentServiceUnreachable featureflags [paymentservice] Add paymentServiceFailure & paymentServiceUnreachable featureflags Feb 15, 2024
@EislM0203 EislM0203 marked this pull request as ready for review February 15, 2024 10:38
@EislM0203 EislM0203 requested a review from a team February 15, 2024 10:38
@EislM0203 EislM0203 marked this pull request as draft February 15, 2024 11:25
@julianocosta89
Copy link
Member

Hey @EislM0203, nice to see you here 👋🏽

Before we check your PR, could you sign the EasyCLA?

@EislM0203
Copy link
Contributor Author

EislM0203 commented Feb 15, 2024

Hey @EislM0203, nice to see you here 👋🏽

Before we check your PR, could you sign the EasyCLA?

Hey there @julianocosta89, its also nice to see you 👋 ! Regarding the CLA, I am working on it

@EislM0203 EislM0203 force-pushed the main branch 2 times, most recently from 2c0f7a4 to 4df9ce8 Compare February 15, 2024 12:18
@EislM0203
Copy link
Contributor Author

EislM0203 commented Feb 15, 2024

Hey @EislM0203, nice to see you here 👋🏽

Before we check your PR, could you sign the EasyCLA?

@julianocosta89 Done 🥳

@EislM0203 EislM0203 marked this pull request as ready for review February 15, 2024 12:31
src/paymentservice/charge.js Outdated Show resolved Hide resolved
src/paymentservice/grpc-ff-client.js Outdated Show resolved Hide resolved
src/paymentservice/grpc-ff-client.js Outdated Show resolved Hide resolved
src/paymentservice/index.js Outdated Show resolved Hide resolved
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.

Looks good, quick question though.

Why are we adding 2 environment variables if the error is the same?
I mean, the msg is different, but all the rest is the same.

docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Member

@austinlparker austinlparker left a comment

Choose a reason for hiding this comment

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

I'll make a similar comment to the one I made for #1382 -- these are fairly blunt failure scenarios (I'm more inclined to favor them since we don't have payment service flags yet iirc) but I'm curious as to the desired observability scenarios we're enabling here.

@EislM0203
Copy link
Contributor Author

@julianocosta89 @austinlparker
You raise valid points. To clarify, I see two interesting observability scenarios for the payment service:

There is an exception with the payment provider
What if the response from the payment provider is an error? What if the request fails? The paymentServiceFailure feature flag covers this scenario by throwing an exception in the charge method after the corresponding span has been started.

For some reason the service is unavailable
As far as I know we currently do not have any feature flag which make an entire service unavailable, which imo is an interesting and valid scenario. I chose a straightforward approach for several reasons. Firstly, I refrained from modifying the underlying Kubernetes resources to make the service unavailable. Secondly, alternatives like dynamically changing the gRPC server port during runtime proved impractical due the way the featureflagservice is implemented in this demo (where 0.5 = 50% activation rate). These methods result in the service being either available or not. The currently implemented approach fails the paymentservice right at the start, if the featureflagservice returns true. This approach not only works quite well with the modus operandi of the featureflagservice but also mimics the behavior of the paymentservice being unavailable.

Also, the main difference in the implementation of the two featureflags is the location in which the error is thrown. For the paymentServiceFailure flag this happens in the charge method after the corresponding span has been started whereas for the paymentServiceUnreachable flag the error is thrown right off the bat when the service is called.

I hope this clears up your questions :)

@puckpuck
Copy link
Contributor

I like the idea of a service being unreachable, thus paymentServiceUnreachable makes sense here.

I struggle to see the value from the paymentServiceFailure flag. We have several feature flags across multiple services that do simple failures similar to this already. If we are adding more feature flags, I would like to see them lead towards different types of failures so we can showcase how to use observability to find them.

@puckpuck
Copy link
Contributor

When I use this fork and enable the paymentServiceUnreachable flag, I still see the Payment service in my traces. If the service is unreachable, I would expect it to fail completely and not respond to any portion of the request, having the error manifest upstream.

Screenshot 2024-02-21 at 10 52 25 PM

Copy link

github-actions bot commented Mar 1, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 1, 2024
@EislM0203
Copy link
Contributor Author

@puckpuck
Hey there, sorry for being a bit unresponsive here. To address your comments:

Regarding the paymentServiceFailure, while I see your point that there are already somewhat similar flags, they perform slightly different functions. The adServiceFailure flag throws an error with a status of resource exhaustion. The cartServiceFailure creates a bad cartstore object (with the wrong redis connection URL) and throws an exception as a result. The most similar feature flag to the one I propose is the productCatalogFailure feature flag. However, even this one affects only a single product and not an entire (integral) feature of a service. Additionally, given that the payment service uses JS, the feature flag would support showcasing how OpenTelemetry handles errors in JS which is not yet available in the OtelDemo. In short, while I understand your perspective, I still believe that an error in the charge method of the payment service is a probable real-world scenario/problem that people would find interesting to address using observability to find the root cause.

Regarding the paymentServiceUnreachable flag, what you are saying makes a lot of sense. The exception occurring at the very beginning when the payment service is called is a result of a compromise, as I refrained from modifying the underlying kubernetes resources to truly render the service unavailable. If you don't like the current implementation, there is one other viable approach: Move the feature flag from the paymentService itself to the checkoutService and, when enabled, update the PAYMENT_SERVICE_ADDR so that it no longer points to the paymentService. Please let me know if this would be a valid approach for you. If yes, I will make the changes.

I hope this answers your questions/comments.

@github-actions github-actions bot removed the Stale label Mar 6, 2024
@julianocosta89
Copy link
Member

@EislM0203 we have just merged a PR that removed the current FeatureFlagService in favour of OpenFeature and FlagD.
Unfortunately your PR needs to be revisited.
Would you be able to update it?

@EislM0203
Copy link
Contributor Author

@julianocosta89 Sure, I will look into it

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.

4 participants