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

Add support for storage class configuration #1714

Merged
merged 11 commits into from
Sep 12, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* [CHANGE] Increase default values for `server.grpc_server_max_recv_msg_size` and `server.grpc_server_max_send_msg_size` from 4MB to 16MB [#1688](https://github.com/grafana/tempo/pull/1688) (@mapno)
* [CHANGE] **BREAKING CHANGE** Use storage.trace.block.row_group_size_bytes to cut rows during compaction instead of
compactor.compaction.flush_size_bytes. [#1696](https://github.com/grafana/tempo/pull/1696) (@joe-elliott)
* [FEATURE] Add capability to configure the used S3 Storage Class [#1697](https://github.com/grafana/tempo/pull/1714) (@amitsetty)
* [ENHANCEMENT] metrics-generator: expose span size as a metric [#1662](https://github.com/grafana/tempo/pull/1662) (@ie-pham)
* [ENHANCEMENT] Set Max Idle connections to 100 for Azure, should reduce DNS errors in Azure [#1632](https://github.com/grafana/tempo/pull/1632) (@electron0zero)
* [ENHANCEMENT] Add PodDisruptionBudget to ingesters in jsonnet [#1691](https://github.com/grafana/tempo/pull/1691) (@joe-elliott)
Expand Down
2 changes: 2 additions & 0 deletions tempodb/backend/s3/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,6 @@ type Config struct {
SignatureV2 bool `yaml:"signature_v2"`
ForcePathStyle bool `yaml:"forcepathstyle"`
Tags map[string]string `yaml:"tags"`
StorageClass string `yaml:"storage_class"`
UserMetadata map[string]string `yaml:"user_metadata"`
amitsetty marked this conversation as resolved.
Show resolved Hide resolved
}
18 changes: 11 additions & 7 deletions tempodb/backend/s3/s3.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,20 @@ func internalNew(cfg *Config, confirm bool) (backend.RawReader, backend.RawWrite
return rw, rw, rw, nil
}

func getTempoObjectOptions(rw *readerWriter) minio.PutObjectOptions {
amitsetty marked this conversation as resolved.
Show resolved Hide resolved
return minio.PutObjectOptions{
PartSize: rw.cfg.PartSize,
UserTags: rw.cfg.Tags,
StorageClass: rw.cfg.StorageClass,
UserMetadata: rw.cfg.UserMetadata,
}
}

// Write implements backend.Writer
func (rw *readerWriter) Write(ctx context.Context, name string, keypath backend.KeyPath, data io.Reader, size int64, _ bool) error {
objName := backend.ObjectFileName(keypath, name)

putObjectOptions := minio.PutObjectOptions{
PartSize: rw.cfg.PartSize,
UserTags: rw.cfg.Tags,
}
putObjectOptions := getTempoObjectOptions(rw)

info, err := rw.core.Client.PutObject(
ctx,
Expand Down Expand Up @@ -138,9 +144,7 @@ func (rw *readerWriter) Append(ctx context.Context, name string, keypath backend
var a appendTracker
objectName := backend.ObjectFileName(keypath, name)

options := minio.PutObjectOptions{
PartSize: rw.cfg.PartSize,
}
options := getTempoObjectOptions(rw)
Copy link
Member

Choose a reason for hiding this comment

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

This does add UserTags here which feels like a bug fix. Can you think of any reason we wouldn't want that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about it a little.
This will just set the user tags to the new user tags during append.
Sounds to me like something that leads to a negligible performance impact and will only lead to a behavior change if someone changes the user tags (which in this case will lead to the new user tags when appending)

Seems to me that if anything this is a good thing unless there is a performance change. Do you think that this could lead to a performance change? If so I'll be more than happy to change it

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree it seems like a great bug fix. It doesn't look like the headers are supported on UploadPart but they are supported on CreateMultipartUpload. Since it's working then either the extra headers don't matter or the minio client is handling it. Have you ran this against real S3 and seen any issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't ran it against a real S3. Feel free to check and I'll make any changes necessary :)

if tracker != nil {
a = tracker.(appendTracker)
} else {
Expand Down
47 changes: 43 additions & 4 deletions tempodb/backend/s3/s3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ import (
"github.com/stretchr/testify/require"
)

const TAGHEADER = "X-Amz-Tagging"
amitsetty marked this conversation as resolved.
Show resolved Hide resolved
const STORAGECLASSHEADER = "X-Amz-Storage-Class"

func TestHedge(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -118,17 +121,17 @@ func TestReadError(t *testing.T) {
assert.Equal(t, wups, errB)
}

func fakeServerWithTags(t *testing.T, obj *url.Values) *httptest.Server {
func fakeServerWithHeader(testedHeaderName string, t *testing.T, obj *url.Values) *httptest.Server {
amitsetty marked this conversation as resolved.
Show resolved Hide resolved
require.NotNil(t, obj)

server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch method := r.Method; method {
case "PUT":
// https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObject.html
switch tagHeaders := r.Header.Get("X-Amz-Tagging"); tagHeaders {
switch testedHeaderValue := r.Header.Get(testedHeaderName); testedHeaderValue {
case "":
default:
value, err := url.ParseQuery(tagHeaders)
value, err := url.ParseQuery(testedHeaderValue)
require.NoError(t, err)
*obj = value
}
Expand Down Expand Up @@ -162,7 +165,7 @@ func TestObjectBlockTags(t *testing.T) {
// rawObject := raw.Object{}
var obj url.Values

server := fakeServerWithTags(t, &obj)
server := fakeServerWithHeader(TAGHEADER, t, &obj)
_, w, _, err := New(&Config{
Region: "blerg",
AccessKey: "test",
Expand All @@ -185,3 +188,39 @@ func TestObjectBlockTags(t *testing.T) {
})
}
}

func TestObjectStorageClass(t *testing.T) {

tests := []struct {
name string
StorageClass string
// expectedObject raw.Object
}{
{
"Standard", "STANDARD",
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
// rawObject := raw.Object{}
var obj url.Values

server := fakeServerWithHeader(STORAGECLASSHEADER, t, &obj)
_, w, _, err := New(&Config{
Region: "blerg",
AccessKey: "test",
SecretKey: flagext.SecretWithValue("test"),
Bucket: "blerg",
Insecure: true,
Endpoint: server.URL[7:], // [7:] -> strip http://
StorageClass: tc.StorageClass,
})
require.NoError(t, err)

ctx := context.Background()
_ = w.Write(ctx, "object", backend.KeyPath{"test"}, bytes.NewReader([]byte{}), 0, false)
amitsetty marked this conversation as resolved.
Show resolved Hide resolved

})
}
}