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

feat: Etag version #3248

Merged
merged 22 commits into from
Jul 12, 2024
Merged

feat: Etag version #3248

merged 22 commits into from
Jul 12, 2024

Conversation

markphelps
Copy link
Collaborator

@markphelps markphelps commented Jul 7, 2024

supersedes: #3240

This takes a different approach to calculating a stable etag for the /evaluation/v1/internal/data API (supports client-side eval)

Since we can't depend on the ordering of the JSON being consistent between requests (discussed in #3240 ), we use a different way to track if any state has been mutated to calculate a stable etag.

For each mutation in the store layer we update a version value (for SQL backends) in the db by setting the current date/time that the data was mutated in that namespace.

We still need to implement this for the non-relational stores, for example the OCI and Git backends can simply return the latest reference. We'll have to also implement this for the object stores. There we could have the store hash the data that it has in memory for a stable reference.

The pros of this approach are that we have full control over when we say that the data has changes from Flipt's perspective, and we can guarantee it is consistent

TODO

  • get buy in on this approach (cc @erka , @GeorgeMac, @thepabloaguilar )
  • implement for remaining relational databases
  • implement for oci store
  • implement for git store
  • implement for object stores
  • add unit tests
  • add integration tests

Copy link

codecov bot commented Jul 7, 2024

Codecov Report

Attention: Patch coverage is 68.93204% with 64 lines in your changes missing coverage. Please review.

Project coverage is 64.12%. Comparing base (e32ca8e) to head (83e0455).

Files Patch % Lines
...al/server/evaluation/data/evaluation_store_mock.go 15.00% 17 Missing ⚠️
internal/server/middleware/http/middleware.go 41.37% 16 Missing and 1 partial ⚠️
internal/storage/sql/common/storage.go 31.81% 15 Missing ⚠️
internal/server/evaluation/data/server.go 80.95% 3 Missing and 1 partial ⚠️
internal/common/store_mock.go 0.00% 3 Missing ⚠️
internal/cmd/http.go 0.00% 2 Missing ⚠️
internal/storage/fs/object/store.go 0.00% 2 Missing ⚠️
internal/storage/fs/snapshot.go 0.00% 2 Missing ⚠️
internal/storage/fs/store.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3248      +/-   ##
==========================================
+ Coverage   64.08%   64.12%   +0.03%     
==========================================
  Files         166      168       +2     
  Lines       13217    13393     +176     
==========================================
+ Hits         8470     8588     +118     
- Misses       4091     4147      +56     
- Partials      656      658       +2     
Flag Coverage Δ
unittests 64.12% <68.93%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@markphelps
Copy link
Collaborator Author

curl -v  http://localhost:8080/internal/v1/evaluation/snapshot/namespace/default
* Host localhost:8080 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080
> GET /internal/v1/evaluation/snapshot/namespace/default HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.6.0
> Accept: */*
> 
< HTTP/1.1 200 OK
< Content-Length: 1401
< Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src * data:; frame-ancestors 'none'; connect-src 'self' https://app.formbricks.com; script-src-elem 'self' https://unpkg.com;
< Content-Type: application/json
< Etag: e8751091a3204af297fb9f917211f79fc4020141
< Grpc-Metadata-Content-Type: application/grpc
< X-Content-Type-Options: nosniff
< Date: Sun, 07 Jul 2024 22:50:00 GMT
< 
* Connection #0 to host localhost left intact
{"namespace":{"key":"default"},"flags":[{"key":"beta-new-ui-framework","name":"beta-new-ui-framework","description":"Testing newest framework of the week","enabled":true,"type":"BOOLEAN_FLAG_TYPE","createdAt":"2024-03-25T18:48:04.717326Z","updatedAt":"2024-07-06T18:16:41.847305Z","rules":[],"rollouts":[]},{"key":"dynamic-pricing-algorithm","name":"dynamic-pricing-algorithm","description":"Enables the testing of new pricing algorithms in real-time","enabled":false,"type":"VARIANT_FLAG_TYPE","createdAt":"2024-03-25T18:48:44.822596Z","updatedAt":"2024-03-25T18:48:44.822596Z","rules":[],"rollouts":[]},{"key":"geo-fenced-content-delivery","name":"geo-fenced-content-delivery","description":"Controls the rollout of region-specific features or content","enabled":true,"type":"BOOLEAN_FLAG_TYPE","createdAt":"2024-03-25T18:49:19.136128Z","updatedAt":"2024-03-25T18:49:22.487783Z","rules":[],"rollouts":[]},{"key":"iot-deviceUpdateControl","name":"iot-deviceUpdateControl","description":"Manages the rollout of updates to our IoT devices","enabled":false,"type":"VARIANT_FLAG_TYPE","createdAt":"2024-03-25T18:50:23.696970Z","updatedAt":"2024-06-05T15:37:56.456298Z","rules":[],"rollouts":[]},{"key":"test","name":"test","description":"foo \"bar\"","enabled":true,"type":"VARIANT_FLAG_TYPE","createdAt":"2024-04-10T14:19:23.029436Z","updatedAt":"2024-04-10T14:19:23.029436Z","rules":[],"rollouts":[]}]}% 

curl -v  -H "If-None-Match: e8751091a3204af297fb9f917211f79fc4020141" http://localhost:8080/internal/v1/evaluation/snapshot/namespace/default
* Host localhost:8080 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080
> GET /internal/v1/evaluation/snapshot/namespace/default HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.6.0
> Accept: */*
> If-None-Match: e8751091a3204af297fb9f917211f79fc4020141
> 
< HTTP/1.1 304 Not Modified
< Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src * data:; frame-ancestors 'none'; connect-src 'self' https://app.formbricks.com; script-src-elem 'self' https://unpkg.com;
< Etag: e8751091a3204af297fb9f917211f79fc4020141

@erka
Copy link
Collaborator

erka commented Jul 8, 2024

It would be nice to have an ETag per namespace. This way, if someone uses a single instance of Flipt for different environments or teams, they won't impact each other. For relational databases, we could use/update namespace.updated_at as the basis for the ETag, eliminating the need for additional table and migration. However, I acknowledge that this approach has its drawbacks.

For object storage, it would be ideal to use the ETag provided by the storage system and avoid extra fetching during pooling as well. gocloud offers ETags in bucket attributes, so it's likely possible to obtain ETags for all supported storage solutions. I could try implement this as separated PR.

wdyt @markphelps ?

@markphelps
Copy link
Collaborator Author

It would be nice to have an ETag per namespace. This way, if someone uses a single instance of Flipt for different environments or teams, they won't impact each other. For relational databases, we could use/update namespace.updated_at as the basis for the ETag, eliminating the need for additional table and migration. However, I acknowledge that this approach has its drawbacks.

For object storage, it would be ideal to use the ETag provided by the storage system and avoid extra fetching during pooling as well. gocloud offers ETags in bucket attributes, so it's likely possible to obtain ETags for all supported storage solutions. I could try implement this as separated PR.

wdyt @markphelps ?

Great idea @erka ! I might suggest we add a new column to the namespaces table instead of relying on updated_at since it has existing meaning. But I love the idea of scoping it to the namespace which means we don't need a new table.

Will implement today.

For object storage, it would be ideal to use the ETag provided by the storage system and avoid extra fetching during pooling as well. gocloud offers ETags in bucket attributes, so it's likely possible to obtain ETags for all supported storage solutions. I could try implement this as separated PR.

Yes maybe once I update the interface to return the value for when any data under a namespace changes then you could add support for the object backends ? Then we can have each of the storage backends implement this interface

@thepabloaguilar
Copy link
Contributor

@markphelps we're gonna need it to use for OFREP endpoints, but it should track not only the namespace as a whole but each flag too!
Maybe an other alternative approach we could add based on your last PR is to order the bytes of the response and generate the Etag from it, this way we avoid the unpredictable map mashalling

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
}

// HandleNoBodyResponse is a response modifier that does not write a body if the response is a 204 No Content or 304 Not Modified.
func HandleNoBodyResponse(next http.Handler) http.Handler {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is required because grpc gateway will always call Write on the responseWriter, even if the status code is a 204 or 304, and the Go stdlib will return the error: Failed to write response: http: request method or response status code does not allow body which we don't want to log as errors in our logs

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@markphelps markphelps marked this pull request as ready for review July 10, 2024 19:40
@markphelps markphelps requested a review from a team as a code owner July 10, 2024 19:40
Co-authored-by: Roman Dmytrenko <rdmytrenko@gmail.com>
@markphelps
Copy link
Collaborator Author

@markphelps we're gonna need it to use for OFREP endpoints, but it should track not only the namespace as a whole but each flag too! Maybe an other alternative approach we could add based on your last PR is to order the bytes of the response and generate the Etag from it, this way we avoid the unpredictable map mashalling

@thepabloaguilar im not sure tracking it at the flag level would work since changes outside of the flag could also affect evaluation. for example if you have a flag that has a rule using a segment. any change to that segment would affect evaluation for that flag, but since it wouldn't change the flag itself then the etag wouldn't change. if that makes sense

Copy link
Contributor

@GeorgeMac GeorgeMac 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. Are you doing the other stores in subsequent PRs?

@markphelps
Copy link
Collaborator Author

Looks good. Are you doing the other stores in subsequent PRs?

yah right now they will just not support etags since they will always return an empty string and the logic states to no set an empty etag, so should be backward compatible. but will implement those next

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Jul 12, 2024
@kodiakhq kodiakhq bot merged commit 4913131 into main Jul 12, 2024
58 checks passed
@kodiakhq kodiakhq bot deleted the etag-version branch July 12, 2024 15:58
erka added a commit to erka/flipt that referenced this pull request Jul 18, 2024
related flipt-io#3248

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
erka added a commit to erka/flipt that referenced this pull request Jul 18, 2024
related flipt-io#3248

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
kodiakhq bot pushed a commit that referenced this pull request Jul 19, 2024
* feat: support etag for declarative stores

related #3248

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* address PR feedback

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* implement etag for object store

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

---------

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants