Skip to content

Conversation

@Kavindu-Dodan
Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan commented Jan 26, 2024

OFREP evaluation APIs

A proposal for the initial version of the OpenAPI specification for the OFREP service.

Please feel free to review, suggest, and propose changes so we can collectively define the service.

Why OpenAPI?

According to the survey results, JSON over HTTP was the most widely used technology by vendors.

What's in this PR?

image

This PR mainly focuses on the flag evaluation. It contains,

  • Endpoint for individual flag evaluation - /ofrep/v1/evaluate/{key}:

This endpoint evaluates an individual feature flag from the flag management system

  • Endpoint for bulk evaluation - /ofrep/v1/evaluate

This endpoint mainly focuses on the static context paradigm and allows multiple flags (bulk) evaluations based on context, authorization token, or flag keys in the request payload.

This PR does NOT focus on telemetry or flag evaluation metrics. This will be a follow-up discussion.

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Copy link
Member

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

I like this interface that is super simple for flag evaluation.

I am just wondering if we don't need extra information in the success schema to help the provider to deal with the result of this flag.

It may be too early, but I am thinking about extra information such as:

  • provider can cache the value
  • or tracking is activated on this flag
  • maybe a version of the flag (even if this could be in the metadata).

Copy link
Member

@nicklasl nicklasl left a comment

Choose a reason for hiding this comment

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

Thanks a lot for kick starting this!
I left a bunch of comments mostly from the static paradigm context :)

@nicklasl nicklasl closed this Jan 31, 2024
@nicklasl nicklasl reopened this Jan 31, 2024
@lukas-reining
Copy link
Member

lukas-reining commented Jan 31, 2024

Hey thanks for kicking it off.
I am wondering if we should already include something like SSE to allow notifying about flag changes etc.
The question would be if it is only about events or even something that can push flag changes to static context providers.
Is this something for later or something that we already want to have included?
Also we might have a mixture between polling and SSEs or even Websockets.
My feeling is that HTTP streaming like SSE would be the best as it is the least complex to use and works in any HTTP environment.

@thomaspoignant
Copy link
Member

Hey thanks for kicking it off.
I am wondering if we should already include something like SSE to allow notifying about flag changes etc.
The question would be if it is only about events or even something that can push flag changes to static context providers.
Is this something for later or something that we already want to have included?
Also we might have a mixture between polling and SSEs or even Websockets.
My feeling is that HTTP streaming like SSE would be the best as it is the least complex to use and works in any HTTP environment.

From the survey results it seems that polling is probably a better option here.
My personal opinion is that we need to find a way in the providers to offer different solutions that can be configured by the feature flag solution.

The provider can probably support something like:

  1. Polling
  2. SSE
  3. Websocket
  4. etc ...

But to start small and based on the survey results I think we should start with polling 1st.

@Kavindu-Dodan
Copy link
Contributor Author

Hey thanks for kicking it off.
I am wondering if we should already include something like SSE to allow notifying about flag changes etc.
The question would be if it is only about events or even something that can push flag changes to static context providers.
Is this something for later or something that we already want to have included?
Also we might have a mixture between polling and SSEs or even Websockets.
My feeling is that HTTP streaming like SSE would be the best as it is the least complex to use and works in any HTTP environment.

From the survey results it seems that polling is probably a better option here. My personal opinion is that we need to find a way in the providers to offer different solutions that can be configured by the feature flag solution.

The provider can probably support something like:

  1. Polling
  2. SSE
  3. Websocket
  4. etc ...

But to start small and based on the survey results I think we should start with polling 1st.

I also think we can first focus on the dynamic context paradigm and then focus on the dynamic context paradigm. But this is something we can collectively decide.

So far I have the following key points for the next session (from a technical stand point)

  1. API definition: Definition language (most probably this will be OpenAPI)
  2. Required endpoints
    • Single endpoint vs multiple endpoint
    • Dynamic ctx vs Static ctx
    • Payload content
  3. Caching and cache-busting requirements
  4. Observability

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
@thomaspoignant
Copy link
Member

thomaspoignant commented Feb 7, 2024

After discussing with @Kavindu-Dodan I've modified the openAPI spec to include some changes, particularly related to the static context paradigm and polling.
Here is a summary of the changes if you want to review them:

  • rename flagKey to key (for consistency with the OF nomenclature)
  • add versioning on the API with a /v1/ prefix
  • added key in the path of the URL for /v1/evaluate/{key} for single flag evaluation
  • Add a 404 error if the flag is not found for single flag evaluation
  • Add a 429 error if the rate limit is reached
  • Add a 500 error if the server is not able to process the request
  • Adding a new endpoint for bulk evaluation /v1/evaluate
    • By default, the bulk evaluation will return the evaluation of all the flags available inside the flag management system except if you provide a list of flags to evaluate.
    • If no flags are provided, filtering which flag will be evaluated will be the responsibility of the flag management system, probably based on the access key used to authenticate the request.
  • Adding a new endpoint for polling flag changes /v1/flag/changes
    • Why polling? Because it seems the most used way of being notified of a flag change based on the survey results
    • This endpoint will return a timestamp with the last time the flags have been changed in the flag management system. It will be the responsibility of the provider to know if they should proceed to a re-evaluation of the flags by calling /v1/evaluate

I've kept the explicit type on /v1/evaluate/{key} because I found it elegant to be explicit on the typing and it allows to delegate all the TYPE_MISMATCH errors to the flag management system.
But when it comes to bulk evaluation we cannot be explicit on the type because we don't necessarily know the flags we will evaluate. So not sure if we should stay consistent for both endpoints 🤷‍♂️.

cc @Kavindu-Dodan @jonathannorris @nicklasl @lukas-reining @beeme1mr @lukas-reining

@liran2000
Copy link
Member

/v1/evaluate/{key}

When the key is in the path, it can involve needing for url encode it for special chars. It can create unexpected behavior in some web server configurations. To overcome it from beginning, what do you think of passing the flag key on the request payload as some Json? It can also make it flexible for adding additional params if needed.

@thomaspoignant
Copy link
Member

/v1/evaluate/{key}

When the key is in the path, it can involve needing for url encode it for special chars. It can create unexpected behavior in some web server configurations. To overcome it from beginning, what do you think of passing the flag key on the request payload as some Json? It can also make it flexible for adding additional params if needed.

I understand your concern here, but having the key in the path is more RESTful, adding it and encoding it, is a classic pattern on how to build APIs so I don't see that as a blocker for the flag management systems.

@lukas-reining
Copy link
Member

lukas-reining commented Feb 8, 2024

I think we should also specify that we expect optional Authorization headers or whatever we want to use.
We will definitely have to have an option in the provider so we might want to just accept any Authorization header and also none.
This would also add 401 and maybe 403 responses.

@Kavindu-Dodan Kavindu-Dodan marked this pull request as ready for review February 8, 2024 22:23
@Kavindu-Dodan Kavindu-Dodan requested a review from a team as a code owner February 8, 2024 22:23
thomaspoignant and others added 5 commits February 29, 2024 18:26
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Thomas Poignant <thomas.poignant@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Lukas Reining <lukas.reining@codecentric.de>
Signed-off-by: Lukas Reining <lukas.reining@codecentric.de>
@lukas-reining
Copy link
Member

lukas-reining commented Mar 1, 2024

ADRs

As discussed in the OFREP meeting and with @Kavindu-Dodan and @thomaspoignant I added ADRs for the decisions that we felt like are important to track and have a record of.

We came up wit the following points to track in an adr:

  • HTTP / JSON
  • Polling over other systems
  • Re-evaluation for each polling
  • No explicit type handling

The structure of ADRs is bases on Michael Nygards template: https://cognitect.com/blog/2011/11/15/documenting-architecture-decisions
For managing ADRs we use https://github.com/npryce/adr-tools.
The ADRs can still be created and managed by hand, the tool just helps creating new ones and linking them so it is optional to use, while the structure of each file and naming should followed.

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@toddbaert
Copy link
Member

Nice work @Kavindu-Dodan and everyone who helped! I'm excited about this. 🚀

@toddbaert toddbaert self-requested a review March 1, 2024 19:50
Kavindu-Dodan and others added 2 commits March 1, 2024 12:46
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Copy link
Member

@nicklasl nicklasl left a comment

Choose a reason for hiding this comment

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

🚀 Good stuff! Love the ADRs!

Copy link

@dyladan dyladan 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 to me. I like having the ADRs as well

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Contributor Author

Update 05.03.2024

Based on feedback, I have done following improvements,

  • Added a dedicated 404 response for single flag evaluation to represent flag not found state
  • Removed TYPE_MISMATCH reason from response as type handling is done by the provider
  • Added optional generalErrorResponse schema for 5xx response with errorDetails property

@Kavindu-Dodan
Copy link
Contributor Author

If there are no more concerns, I will merge this version of the OpenAPI specification on 07.03.2024.

Once merged, we could continue to improve the API through issues and follow-up PRs. I expect such changes to be non-breaking so that OFREP adaptions can proceed smoothly.

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.