user-configurable overrides: handle versioning, add PATCH endpoint #2681
Conversation
| // ConfirmVersioning is enabled when creating the backend client. If versioning is disabled no | ||
| // checks against concurrent writes will be performed. | ||
| ConfirmVersioning bool `yaml:"confirm_versioning"` |
There was a problem hiding this comment.
I considered placing this on gcs.Config instead but I'm worried this would conflict with our existing bucket configuration. For storing traces you don't need versioning, so setting this to default true would cause a lot of issues.
There was a problem hiding this comment.
Agree with the reasoning here 👍
| if err != nil { | ||
| return | ||
| return nil, "", err | ||
| } |
There was a problem hiding this comment.
This is a change in behaviour: instead of returning nil on not found just pass through the backend.ErrDoesNotExist error. I thought this would be easier and make the client more of a simple passthrough component.
| @@ -198,6 +211,44 @@ func (rw *readerWriter) ReadRange(ctx context.Context, name string, keypath back | |||
| func (rw *readerWriter) Shutdown() { | |||
| } | |||
|
|
|||
| func (rw *readerWriter) WriteVersioned(ctx context.Context, name string, keypath backend.KeyPath, data io.Reader, version backend.Version) (backend.Version, error) { | |||
| // TODO use conditional if-match API | |||
There was a problem hiding this comment.
To properly implement this we need to upgrade our Azure SDK from azure-storage-blob-go to the newer azure-sdk-for-go. Since this impacts the entire package I'm going to do this in a separate PR...
…g-patch # Conflicts: # CHANGELOG.md # cmd/tempo/app/modules.go # integration/e2e/overrides_test.go # modules/overrides/userconfigurableapi/client.go # modules/overrides/userconfigurableapi/http.go # modules/overrides/userconfigurableapi/http_test.go
There was a problem hiding this comment.
This looks pretty good to me. One comment about the signatures in a couple spots. I was reminded the other day while digging around in tempodb, that there are some good reasons to accept interfaces as arguments, and return structs. I think it can make working with those objects a little easier in some cases, like testing. "accept interfaces, return structs" goes back a ways in google, but I don't know the source. Anyway, maybe worth considering or some conversation. What do you think?
tempodb/backend/versioned.go
Outdated
|
|
||
| var _ VersionedReaderWriter = (*fakeVersionedReaderWriter)(nil) | ||
|
|
||
| func NewFakeVersionedReaderWriter(r RawReader, w RawWriter) VersionedReaderWriter { |
There was a problem hiding this comment.
Consider returning the struct instead of the interface.
There was a problem hiding this comment.
That's a good tip that I almost always forget about. I have the tendency to hide abstractions from my Java days, but I agree returning the struct gives more flexibility to the caller since they can create their own interface around it.
For this specific example: fakeVersionedReaderWriter is private and returning it gives me this warning in GoLand. Not sure if making it public or just returning the interface would be best here? 🤔😅
There was a problem hiding this comment.
I think making it public is okay.
| // ConfirmVersioning is enabled when creating the backend client. If versioning is disabled no | ||
| // checks against concurrent writes will be performed. | ||
| ConfirmVersioning bool `yaml:"confirm_versioning"` |
There was a problem hiding this comment.
Agree with the reasoning here 👍
…g-patch # Conflicts: # modules/overrides/userconfigurableapi/client.go # tempodb/backend/gcs/gcs.go
What this PR does:
Add version handling and a new PATCH endpoint to the user-configurable overrides. Existing endpoints gain support for conditional headers adding protection against data races.
New PATCH endpoint: instead of providing the full limits struct, clients can send a partial json with the other fields set to null or omitted. End result: Tempo will read the existing user-configurable overrides and only updates the fields that are set in the patch.
Changes to GET, POST and DELETE:
GET and POST will return the current version of the overrides using the
Etagheader.POST and DELETE must have the
If-Matchhedaer containing the version of a previous request. This ensures the overrides haven't been modified since the last GET.backend.VersionedReaderWriteris a new interface to add version handling to backends. While GCS and Azure support conditional headers, S3 does not. Instead we adopt a best effort mechanism in which we fetch the current version and check it before writing.Implementation:
Which issue(s) this PR fixes:
Fixes #Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]