Update cart service feature flag to match documented 10% failure

[chook]

Matching the behavior documented in https://opentelemetry.io/docs/demo/feature-flags/ that both ad service and cart service failures are happening 10% of the time.

Changes

Please provide a brief description of the changes here.

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.

[julianocosta89]

I'm approving because this is what is happening indeed.

But TBH this should be a bug in the FlagD project.

If the defaultVariant is set to off, I wouldn't expect this case to be enabled at all.

@beeme1mr do you know if there is anything being discussed in the community to solve this?

[beeme1mr]

I'm approving because this is what is happening indeed.

But TBH this should be a bug in the FlagD project.

If the defaultVariant is set to off, I wouldn't expect this case to be enabled at all.

@beeme1mr do you know if there is anything being discussed in the community to solve this?

This is working as designed. The defaultVariant is only used if no matches are returned from the targeting rule. In this case, the "fractional" operation will always return either "on" or "off". Please note that this change will cause ~10% of unique sessions to see the "on" variant (which throws in the demo app). You can change the "state" property to "DISABLED" if you want this flag to be ignored.

[julianocosta89]

Ah, wait, now I'm confused.

This is an issue on the docs, not on the demo.

The behaviour that I was referring was the one in AdService, that it is failing 10% by default.

@chook, would you mind updating the docs instead of the feature flag here?

[julianocosta89]

I'd rather have this updated in the docs, than in here.
By default we shouldn't have any error at start.

[chook]

nothing is failing by default. only if you enable the feature flag.

If you prefer to update the docs to show that cart will fail all the time and not tentatively (10-90), i'm game. Just wanted to contribute <3

[julianocosta89]

@chook that's exactly the problem that I was telling @beeme1mr.
If we add targeting.fractional the enable/disable is just ignored, and this is like that on purpose.

In other words, if we merge your PR, the cartservice will start failing 10%, even with the defaultVariant being set to off.

[beeme1mr]

Hey @julianocosta89, I think this is what you're looking for.

"cartServiceFailure": {
  "description": "Fail cart service",
  // This will disable the flag. The SDK will use the default value set in the code. Which in this case is `false`.
  // Valid states are "ENABLED" and "DISABLED"
  "state": "DISABLED",
  // Optional: The variant names could be changed to something like "fail" and "succeed".
  "variants": {
    "on": true,
    "off": false
  },
  "targeting": {
    // This will always return a value.
    "fractional": [
      {
        "var": "session"
      },
      [ "on", 10],
      [ "off", 90]
    ]
  },
  // The default variant is basically a fallback in case the targeting rule doesn't return a value.
  "defaultVariant": "off"
},

I believe you'll want to update the flag configuration based on the suggestion above and update the documentation to talk about changing the state instead of the default variant.

[julianocosta89]

Yes!!!!!!
Thank you @beeme1mr !
That's exactly what I was looking for.

[julianocosta89]

@chook would you mind updating your PR to add the "state": "DISABLED" that @beeme1mr mentioned?

[chook]

@julianocosta89 this means we probably want to turn to DISABLED for the ad service as well, as it has the same issue I believe? cc @beeme1mr

[julianocosta89]

Yes!

Personally, I'd get rid of the defaultVariant and just use the state.

[beeme1mr]

Yes, I would recommend changing both to "DISABLED" and updating the docs accordingly.

@julianocosta89, defaultVariant is a required property even though it isn't used in this situation.

[chook]

i wonder if at this point we should continue this here or someone that is more familiar with flagd do a sweep to make the config/doc in better standard

[julianocosta89]

Sure, I just didn't want to overwrite your PR.

[github-actions]

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