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

[FEATURE] Make offline file usage an own provider type #1504

Open
aepfli opened this issue Jan 9, 2025 · 8 comments
Open

[FEATURE] Make offline file usage an own provider type #1504

aepfli opened this issue Jan 9, 2025 · 8 comments
Labels
enhancement New feature or request

Comments

@aepfli
Copy link
Member

aepfli commented Jan 9, 2025

Requirements

Currently, we have two modes: RPC and in-process, and in-process has two different sources: GRPC and file. Initializing in-process GRPC uses many settings, such as host, port, certPath, socket path, connection settings, etc. In contrast, the in-process file provider only needs a file and a polling interval. This confuses users, and the difference for those settings is not 100% clear. We could simplify the API surface and introduce a third provider type, "FILE," representing the offline path.

Proposal

A file-based resolution must be implicitly in-process (It doesn't connect to anything). It needs a minimum of settings, and we can improve our testing significantly. We also want to run all kinds of evaluations, events, and mismatch tests for the file version, making those implementations and implications more transparent.

Add another ProviderType called "FILE".
This provider only needs 4 properties:

  • deadlineMs
  • retryGracePeriod
  • offlineFlagSourcePath (maybe worth renaming to filePath)
  • offlinePollIntervalMs (maybe worth renaming to filePollIntervalMs)

new Config table

Option name Environment variable name Explanation Type & Values Default Compatible resolver
resolver FLAGD_RESOLVER mode of operation String - rpc, in-process rpc rpc & in-process
host FLAGD_HOST remote host String localhost rpc & in-process
port FLAGD_PORT remote port int 8013 (rpc), 8015 (in-process) rpc & in-process
targetUri FLAGD_TARGET_URI alternative to host/port, supporting custom name resolution string null rpc & in-process
tls FLAGD_TLS connection encryption boolean false rpc & in-process
socketPath FLAGD_SOCKET_PATH alternative to host port, unix socket String null rpc & in-process
certPath FLAGD_SERVER_CERT_PATH tls cert path String null rpc & in-process
deadlineMs FLAGD_DEADLINE_MS deadline for unary calls, and timeout for initialization int 500 rpc & in-process & file
streamDeadlineMs FLAGD_STREAM_DEADLINE_MS deadline for streaming calls, useful as an application-layer keepalive int 600000 rpc & in-process
retryBackoffMs FLAGD_RETRY_BACKOFF_MS initial backoff for stream retry int 1000 rpc & in-process
retryBackoffMaxMs FLAGD_RETRY_BACKOFF_MAX_MS maximum backoff for stream retry int 120000 rpc & in-process
retryGracePeriod FLAGD_RETRY_GRACE_PERIOD period in seconds before provider moves from STALE to ERROR state int 5 rpc & in-process & file
keepAliveTime FLAGD_KEEP_ALIVE_TIME_MS http 2 keepalive long 0 rpc & in-process
cache FLAGD_CACHE enable cache of static flags String - lru, disabled lru rpc
maxCacheSize FLAGD_MAX_CACHE_SIZE max size of static flag cache int 1000 rpc
selector FLAGD_SOURCE_SELECTOR selects a single sync source to retrieve flags from only that source string null in-process
offlineFlagSourcePath FLAGD_OFFLINE_FLAG_SOURCE_PATH offline, file-based flag definitions, overrides host/port/targetUri string null in-process file
offlinePollIntervalMs FLAGD_OFFLINE_POLL_MS poll interval for reading offlineFlagSourcePath int 5000 in-process file
contextEnricher - sync-metadata to evaluation context mapping function function identity function in-process

ensure backward compatibility

As some providers already implement offline file paths, we could implement logic that sets the provider type to FILE when the property for offline_source_file is set. This should be fairly easy to achieve, as we have good abstraction in most places.

if offlineFlagSourcePath is set:
    providerType = "FILE"

wdyt?

@aepfli aepfli added enhancement New feature or request Needs Triage This issue needs to be investigated by a maintainer labels Jan 9, 2025
@aepfli aepfli changed the title [FEATURE] Make offline file usage an own provider [FEATURE] Make offline file usage an own provider type Jan 9, 2025
@beeme1mr
Copy link
Member

How much effort do you think this will be? It sounds relatively useful but I'm not sure it's worth the effort.

@aepfli
Copy link
Member Author

aepfli commented Jan 17, 2025

open-feature-forking/java-sdk-contrib#1 this pr shows the efforts for java, with backwards compatibility

@aepfli
Copy link
Member Author

aepfli commented Jan 19, 2025


  @file
  Scenario Outline: File Backwards compatibility
    Given an option "resolver" of type "ResolverType" with value "<resolverSet>"
    And an option "offlineFlagSourcePath" of type "String" with value "some-path"
    When a config was initialized
    Then the option "resolver" of type "ResolverType" should have the value "<resolverExpected>"
    Scenarios:
        | resolverSet | resolverExpected |
        | in-process  | file             |
        | rpc         | rpc              |
        | file        | file             |

  @file
  Scenario: Default Config File
    Given an option "resolver" of type "ResolverType" with value "file"
    When a config was initialized
    Then we should have an error

possible gherkin tests to ensure this behaviour, throughout all our providers.

@aepfli
Copy link
Member Author

aepfli commented Jan 19, 2025

i created a fully fledged demo for java - with gherkin testbed tests, to show how this looks like for java. i do think, that this is a easy to do change, which allows us to treat those providers and logics seperately:

@toddbaert
Copy link
Member

How much effort do you think this will be? It sounds relatively useful but I'm not sure it's worth the effort.

open-feature-forking/java-sdk-contrib#1 this pr shows the efforts for java, with backwards compatibility

This seems like a pretty small change:

Image

@toddbaert
Copy link
Member

I'm definitely in favor of this. Here is my simple reasoning:

  • @aepfli has proved the implementation is pretty low-effort
  • we've taking the time to make the file source work well (initially it was just used for testing) but due to complicated naming, I think it's extremely hard to discover
  • the new configuration clarifies the intent quite a bit

@toddbaert toddbaert removed the Needs Triage This issue needs to be investigated by a maintainer label Jan 20, 2025
@aepfli
Copy link
Member Author

aepfli commented Jan 21, 2025

so i can move forward with the migration?

@beeme1mr
Copy link
Member

so i can move forward with the migration?

Yes, I'm okay with this change. Thanks for the proposal and Java example.

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

No branches or pull requests

3 participants