Skip to content

feat: OpenAPI proposal for V1 OFREP#2

Merged
Kavindu-Dodan merged 36 commits into
mainfrom
feat/openapi-proposal
Mar 7, 2024
Merged

feat: OpenAPI proposal for V1 OFREP#2
Kavindu-Dodan merged 36 commits into
mainfrom
feat/openapi-proposal

Conversation

@Kavindu-Dodan

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

Copy link
Copy Markdown
Contributor

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>

@thomaspoignant thomaspoignant left a comment

Copy link
Copy Markdown
Member

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).

@nicklasl nicklasl left a comment

Copy link
Copy Markdown
Member

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 :)

Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml
Comment thread service/openapi.yaml
Comment thread service/openapi.yaml Outdated
@nicklasl nicklasl closed this Jan 31, 2024
@nicklasl nicklasl reopened this Jan 31, 2024
@lukas-reining

lukas-reining commented Jan 31, 2024

Copy link
Copy Markdown
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.

@thomaspoignant

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml
Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml Outdated
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
@thomaspoignant

thomaspoignant commented Feb 7, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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.

Comment thread service/openapi.yaml
Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml Outdated
@lukas-reining

lukas-reining commented Feb 8, 2024

Copy link
Copy Markdown
Member

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.

Comment thread service/openapi.yaml
@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
Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml Outdated
Kavindu-Dodan and others added 4 commits February 29, 2024 10:24
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

lukas-reining commented Mar 1, 2024

Copy link
Copy Markdown
Member

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>
Comment thread service/adrs/0002-two-evaluation-endpoints.md Outdated
Comment thread service/adrs/0003-no-flag-type-in-request-or-response.md Outdated
@toddbaert

Copy link
Copy Markdown
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>

@nicklasl nicklasl left a comment

Copy link
Copy Markdown
Member

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!

@dyladan dyladan left a comment

Copy link
Copy Markdown

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

Comment thread service/openapi.yaml
Comment thread service/openapi.yaml
Comment thread service/openapi.yaml Outdated
Comment thread service/openapi.yaml Outdated
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan

Copy link
Copy Markdown
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
Copy Markdown
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.

10 participants