feat(flagd): introduce fatalStatusCodes option

[leakonvalinka]

This PR

• adds the fatalStatusCode option + env variable

Related Issues

resulted from this issue

Notes

I'm not too happy with how the fatal error is communicated through the different components (received at SyncStreamQueueSource -> FlagStore -> InProcessResolver -> FlagdProvider, respective RpcResolver -> FlagdProvider). It "misuses" the STALE state to differentiate between normal errors and fatal errors. I couldn't find a cleaner solution for this unfortunately, so feedback on this would be highly appreciated!

Will work on the remaining failing tests once we agree on how to proceed!

Follow-up Tasks

How to test

[chrfwow]

Since we do not want to introduce breaking changes into the api by adding a PROVIDER_FATAL type to ProviderEvent, I have two suggestions how we might be able to work around the "misuse" of the stale event:
We could add a isFatal flag to the FlagdProviderEvent to track the type of error. I don't really like it because this could also be set when the event is not an error event, and with this we split up information that should be stored in one place into two places.
Or, we create an enum class ExtendedProviderEvent, which is a copy of ProviderEvent (enums cannot be extended in Java), plus the additional PROVIDER_FATAL field. We would then have to map where needed between the two types (not 100% sure if this will work). I don't like this either, because we would duplicate the ProviderEvent enum

[chrfwow]

Since we do lots of .contains operation on this data structure, a HashSet might be more performant. How many entries do we expect in this list?

[leakonvalinka]

That's hard for me to estimate, what do the others think? The currently defined default is an empty list

[chrfwow]

We should print an info/warn that the env vars are invalid

[leakonvalinka]

Just for this method? Or the other ones too? I'd either leave it or add it in all cases to be consistent

[chrfwow]

Then we should add it everywhere, but in a different PR

[leakonvalinka]

Alright, sounds good. Should we create a new issue for this or is that overkill?

[guidobrei]

I'm not too happy with how the fatal error is communicated through the different components (received at SyncStreamQueueSource -> FlagStore -> InProcessResolver -> FlagdProvider...)

This is an implication of our provider design and there is not really something to do about that (in this PR).

[toddbaert]

Could we simply add a FATAL storage state to resolve this conceptual "STALE" overloading? This is an entirely private enum, so we can add to it without issue.

[leakonvalinka]

Yes, we could, but this would only solve the missuse issue in the communication step from FlagStore -> InProcessResolver (and not InProcessResolver -> FlagdProvider)
Also, nit pick: StorageState.ERROR is already defined as /** Storage is in an unrecoverable error stage. */, which models what FATAL means for us i think

[guidobrei]

Based on the StorageState existing docs the states are used now as intended:

STALE: Storage has gone stale (most recent sync failed). May get to OK status with next sync.

QueueSource encountered an ERROR but will try to recover.

ERROR: Storage is in an unrecoverable error stage.

QueueSource encountered an ERROR AND will not try to recover. It exited the sync loop.

[toddbaert]

We fixed this by propagating the provider details as well though the consumer, so we don't need to overload or abuse the enums anymore.

[toddbaert]

My biggest question with this whole concept (not your implementation) is whether or not we should care about whether this is the initial sync or not. I'm actually leaning towards "not"... here is my reasoning (anyone feel free to disagree):

• users already fully control what's considered FATAL; they can also control whether or not to consider FATAL at init different than FATAL later, using event handlers and the details of the exception
• it's simpler (less conditions/state to handler in our code [this field would disappear] and for users to understand)

WDYT?

[leakonvalinka]

If it's easy to get the same behaviour through event handlers, I think that might be better, because it allows for more customization. I get both sides, that one might not want to completely shut down if a valid flag config was previously received, but also that one might not want to work with stale data given that a non-transient error was received

[guidobrei]

I agree with @toddbaert that fatality does not depend on being the first sync or not.

However, fatality from the SyncStreamQueueSource's perspective depends on whether we break out the sync loop and stop the sync thread (FATAL ERROR), or continue to reconnect (ERROR).

[toddbaert]

Since we do not want to introduce breaking changes into the api by adding a PROVIDER_FATAL type to ProviderEvent, I have two suggestions how we might be able to work around the "misuse" of the stale event: We could add a isFatal flag to the FlagdProviderEvent to track the type of error. I don't really like it because this could also be set when the event is not an error event, and with this we split up information that should be stored in one place into two places. Or, we create an enum class ExtendedProviderEvent, which is a copy of ProviderEvent (enums cannot be extended in Java), plus the additional PROVIDER_FATAL field. We would then have to map where needed between the two types (not 100% sure if this will work). I don't like this either, because we would duplicate the ProviderEvent enum

I could be missing something, but I don't think this is an issue. The "fatalness" (fatality?) of an event is not communicated by the event type, but the error code associated with the event: https://github.com/open-feature/java-sdk/blob/main/src/main/java/dev/openfeature/sdk/ProviderEventDetails.java#L16. All error events are events. Some error events contain with FATAL error codes.

This is basically what's in Go, as well, right @alexandraoberaigner ?

[leakonvalinka]

I could be missing something, but I don't think this is an issue. The "fatalness" (fatality?) of an event is not communicated by the event type, but the error code associated with the event: https://github.com/open-feature/java-sdk/blob/main/src/main/java/dev/openfeature/sdk/ProviderEventDetails.java#L16. All error events are events. Some error events contain with FATAL error codes.

The issue is that the onProviderEvent method expects a FlagdProviderEvent, which doesn't allow a differentiation between different error types (based on the ProviderEvent enum), but we need to tell apart "normal" errors from fatal ones. My implementation does this by its STALE misuse, which isn't a super clean approach. @chrfwow's suggestions would be 2 alternatives, but also not 100% clean.

Personally I think I prefer adding a FATAL to the StorageState enum (even though this nit pick here) and adding an isFatal flag to the FlagdProviderEvent, but as this also has its disadvantages, so I'd like to hear other opinions as well! :)

[alexandraoberaigner]

I could be missing something, but I don't think this is an issue. The "fatalness" (fatality?) of an event is not communicated by the event type, but the error code associated with the event: https://github.com/open-feature/java-sdk/blob/main/src/main/java/dev/openfeature/sdk/ProviderEventDetails.java#L16. All error events are events. Some error events contain with FATAL error codes.
This is basically what's in Go, as well, right @alexandraoberaigner ?

Yes, in Go its a provider event with error code: see ProviderInitError with ErrorCode including PROVIDER_FATAL

Just found a test in the java-sdk that says how the fatal error should look like: see here

ProviderEvent.PROVIDER_ERROR,
ProviderEventDetails.builder()
                        .errorCode(ErrorCode.PROVIDER_FATAL)
                        .build()

[leakonvalinka]

Just found a test in the java-sdk that says how the fatal error should look like: see here

ProviderEvent.PROVIDER_ERROR,
ProviderEventDetails.builder()
                       .errorCode(ErrorCode.PROVIDER_FATAL)
                       .build()

Yup, I also implemented it like this when the FlagdProvider emits the fatal error here
But that doesnt change anything regarding the question how to cleanly propagate the error through the layers until it reaches the FlagdProvider class, unless I am missing something

[toddbaert]

I made a few small test fixes.

Looks good to me, and FATAL works as expected. One thing to note is that as discussed in various places, we DO NOT act differently on FATAL status codes depending on whether this is the initial connection or not; if the stream sends a FATAL code at any time, the provider transitions to FATAL.

See: open-feature/flagd#1818 (comment)

[toddbaert]

I had to make this test tolerate a small retry/timeout here (it worked most of the time but was flaky for both in-process and RPC)

I think that there's a small lag time between when we fire events, and when the SDK updates the client status. We should consider whether this is a small bug with the SDK, and if we should guarantee that the state is updated before the event handlers run (or not)

@guidobrei @aepfli @leakonvalinka