user-configurable overrides: handle versioning, add PATCH endpoint by yvrhdn · Pull Request #2681 · grafana/tempo

yvrhdn · 2023-07-20T16:34:11Z

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 Etag header.

GET http://localhost:3200/api/overrides

HTTP/1.1 200 OK
Content-Type: application/json
Etag: 123

POST and DELETE must have the If-Match hedaer containing the version of a previous request. This ensures the overrides haven't been modified since the last GET.

backend.VersionedReaderWriter is 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:

GCS: All good ✅
S3: as good as we can get it without API support or external dependencies
Azure: TODO ❌ reading versions and setting if-match headers requires us to update to the new SDK, I will do this in a separate PR

Which issue(s) this PR fixes:
~~Fixes #~~

Checklist

Tests updated
Documentation added TODO https://github.com/grafana/tempo-squad/issues/324
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

yvrhdn · 2023-07-20T19:32:28Z

+	// 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"`


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.

Agree with the reasoning here 👍

yvrhdn · 2023-07-20T19:34:02Z

 	if err != nil {
-		return
+		return nil, "", err
 	}


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.

yvrhdn · 2023-07-20T19:37:25Z

 }

+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


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

zalegrala

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?

zalegrala · 2023-07-25T18:36:51Z

+
+var _ VersionedReaderWriter = (*fakeVersionedReaderWriter)(nil)
+
+func NewFakeVersionedReaderWriter(r RawReader, w RawWriter) VersionedReaderWriter {


Consider returning the struct instead of the interface.

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? 🤔😅

I think making it public is okay.

mapno

LGTM. Just left a few minor comments.

mapno · 2023-07-26T09:29:50Z

+	// 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"`


Agree with the reasoning here 👍

…g-patch # Conflicts: # modules/overrides/userconfigurableapi/client.go # tempodb/backend/gcs/gcs.go

Koenraad Verheyden added 4 commits July 20, 2023 18:21

user-configurable overrides: handle versioning, add PATCH endpoint

ed5ebd3

Add DeleteVersioned; make fmt

e17e6d8

make fmt - file must have missed previous CI checks

870618a

linting

c5f95e6

yvrhdn marked this pull request as ready for review July 20, 2023 17:18

yvrhdn requested review from annanay25, electron0zero, ie-pham, joe-elliott, mapno, mdisibio, stoewer and zalegrala as code owners July 20, 2023 17:18

This was referenced Jul 20, 2023

Validate user-configurable overrides #2682

Merged

user-configurable overrides: add PATCH endpoint #2662

Closed

Koenraad Verheyden added 2 commits July 20, 2023 20:39

Update CHANGELOG.md

6543024

Fix integration tests, smash some bugs

c63453c

yvrhdn commented Jul 20, 2023

View reviewed changes

Koenraad Verheyden added 2 commits July 24, 2023 18:30

make fmt

f5880f0

zalegrala reviewed Jul 25, 2023

View reviewed changes

mapno approved these changes Jul 26, 2023

View reviewed changes

yvrhdn mentioned this pull request Jul 26, 2023

Correctly implement VersionedReaderWriter on Azure #2705

Closed

Koenraad Verheyden added 5 commits July 26, 2023 15:59

Address review comments: log warning on S3/Azure, return structs

e6aef82

Merge branch 'main' into kvrhdn/user-configurable-overrides-versionin…

f6d419e

…g-patch # Conflicts: # modules/overrides/userconfigurableapi/client.go # tempodb/backend/gcs/gcs.go

Fix merge, fix logging

a8b7a1f

linting

89789d6

Export FakeVersionedReaderWriter

a518688

yvrhdn enabled auto-merge (squash) July 26, 2023 18:47

yvrhdn merged commit 56bb981 into grafana:main Jul 26, 2023

yvrhdn deleted the kvrhdn/user-configurable-overrides-versioning-patch branch July 26, 2023 19:06

yvrhdn mentioned this pull request Aug 2, 2023

PATCH requests are being terminated by server #2756

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

user-configurable overrides: handle versioning, add PATCH endpoint#2681

user-configurable overrides: handle versioning, add PATCH endpoint#2681
yvrhdn merged 13 commits intografana:mainfrom
yvrhdn:kvrhdn/user-configurable-overrides-versioning-patch

yvrhdn commented Jul 20, 2023 •

edited

Loading

Uh oh!

yvrhdn Jul 20, 2023

Uh oh!

mapno Jul 26, 2023

Uh oh!

yvrhdn Jul 20, 2023

Uh oh!

yvrhdn Jul 20, 2023

Uh oh!

yvrhdn Jul 26, 2023

Uh oh!

zalegrala left a comment

Uh oh!

zalegrala Jul 25, 2023

Uh oh!

yvrhdn Jul 26, 2023

Uh oh!

zalegrala Jul 26, 2023

Uh oh!

mapno left a comment

Uh oh!

Uh oh!

mapno Jul 26, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		var _ VersionedReaderWriter = (*fakeVersionedReaderWriter)(nil)

		func NewFakeVersionedReaderWriter(r RawReader, w RawWriter) VersionedReaderWriter {

Conversation

yvrhdn commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zalegrala left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yvrhdn commented Jul 20, 2023 •

edited

Loading