Replace TenantIndex, BlockMeta and CompactedBlockMeta with proto near-equivalents #4072
Conversation
zalegrala
force-pushed
the
jsonpbTenantIndex
branch
3 times, most recently
from
513e33b
to
3c10c0f
Compare
zalegrala
changed the title
zalegrala
force-pushed
the
jsonpbTenantIndex
branch
from
a367d65
to
ad6e38b
Compare
zalegrala
force-pushed
the
jsonpbTenantIndex
branch
from
f65adc7
to
cc764f5
Compare
| @@ -222,7 +222,8 @@ PROTOC = docker run --rm -u ${shell id -u} -v${PWD}:${PWD} -w${PWD} ${DOCKER_PRO | |||
| PROTO_INTERMEDIATE_DIR = pkg/.patched-proto | |||
| PROTO_INCLUDES = -I$(PROTO_INTERMEDIATE_DIR) | |||
| PROTO_GEN = $(PROTOC) $(PROTO_INCLUDES) --gogofaster_out=plugins=grpc,paths=source_relative:$(2) $(1) | |||
| PROTO_GEN_WITH_VENDOR = $(PROTOC) $(PROTO_INCLUDES) -Ivendor -Ivendor/github.com/gogo/protobuf --gogofaster_out=plugins=grpc,paths=source_relative:$(2) $(1) | |||
| PROTO_GEN_WITH_VENDOR = $(PROTOC) $(PROTO_INCLUDES) -Ivendor -Ivendor/github.com/gogo/protobuf -Ipkg/tempopb --gogofaster_out=plugins=grpc,paths=source_relative:$(2) $(1) | |||
pkg/tempopb/tempo.proto
Outdated
| @@ -2,6 +2,8 @@ syntax = "proto3"; | |||
|
|
|||
| package tempopb; | |||
|
|
|||
| option go_package = "github.com/grafana/tempo/pkg/tempopb"; | |||
pkg/uuid/uuid.go
Outdated
| @@ -0,0 +1,87 @@ | |||
| // Package uuid provides a UUID type that can be used in protocol buffer | |||
There was a problem hiding this comment.
can we move this to tempodb/backend? it would resolve a lot of the google_uuid vs uuid conflict
tempodb/backend/encoding.go
Outdated
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
There was a problem hiding this comment.
for perf: data[0] = byte(e)
| return nil, nil | ||
| } | ||
|
|
||
| return json.Marshal(dcs) |
There was a problem hiding this comment.
let's add a comment about why we are doing this add and the dedicated columns "interning"
| // TenantIndex implements backend.Reader | ||
| func (r *reader) TenantIndex(ctx context.Context, tenantID string) (*TenantIndex, error) { | ||
| reader, size, err := r.r.Read(ctx, TenantIndexName, KeyPath([]string{tenantID}), nil) | ||
| ctx, span := tracer.Start(ctx, "reader.TenantIndex") |
There was a problem hiding this comment.
after our discussions yesterday and some thought i'm more on board with leaving the individual block metas json, and only swapping the tenant index over to proto.
i think this would be accomplished by removing all the special backend logic as well as the above blockMetaPB function. Allow that code to take your new BlockMeta struct and marshal to/from json as always.
this would be a huge win for resource usage in compactors/queriers/frontends and dodge the more challenging elements of this PR.
| } | ||
|
|
||
| out := &TenantIndex{} | ||
| err = out.Unmarshal(bytesPb) |
There was a problem hiding this comment.
perhaps make this a method on the tenant index itself to mirror the existing marshal/unmarshal?
https://github.com/grafana/tempo/blob/main/tempodb/backend/tenantindex.go#L58
also, it looks like we have no compression here? i think we should attempt to use zstd as a replacement for gzip.
What this PR does:
Here we replace the
TenantIndex,BlockMetaandCompactedBlockMetawith structs which are generated from proto. This allows us to convert the backend to proto-only in the future without the translation between a backend struct and an internal struct, since the internal structs here are being replaced.The replacement structs are not identical. Of note, the
BlockMetastruct has several fields which have no proto equivalent. In order to overcome this, several types have been defined in the proto asbytes, which allows the use of thecustomtypegogo proto option to point to a specific type. Those types must conform to a specific interface, and so methods have been added where necessary.backend.DedicatedColumnsupdated to include proto marshal and unmarshal methodsbackend.Encodingupdated to include prto marshal and unmarshal methodsbackend.BlockMetahas a few type changesTotalObjectschangesint->int32Sizebecomes a funcion and the old size is moved toSize_CompactionLevelchangesuint8->uint32BloomShardCountchangesuint16->uint32ReplicationFactorchangesuint8->uint32A new UUID package has been added to wrap the google UUID package and provide
the necessary interface for use with proto.
Currently for all three files (
TenantIndex,BlockMeta, andCompactedBlockMeta), each follow the same logic:This will allow us to read old blocks that were written before this change, and also roll back if necessary. In a future release, we can stop writing the json to the backend and only read/write proto.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]