grpcutil: new+fast FromIncomingContext variant #96318

knz · 2023-01-31T23:24:06Z

If we want to properly leverage gRPC metadata, we need to find a way
around the fact that the default metatada.FromIncomingContext is
slow and expensive. This patch fixes it.

BenchmarkFromIncomingContext/stdlib-32          10987958               327.5 ns/op           432 B/op          3 allocs/op
BenchmarkFromIncomingContext/fast-32            698772889                5.152 ns/op           0 B/op          0 allocs/op

Epic: None

knz · 2023-01-31T23:24:15Z

@andreimatei for your consideration.

cockroach-teamcity · 2023-01-31T23:24:16Z

This change is

andreimatei

I like the functions that read the metadata without copying it.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @benbardin and @knz)

pkg/util/grpcutil/fast_metadata.go line 40 at r1 (raw file):

// from the given metadata key, if it exists. No extra objects are
// allocated. The key is assumed to contain only ASCII characters.
func FastFirstValueFromIncomingContext(ctx context.Context, key string) (bool, string) {

I think it's pretty broad convention for the bool return value to be the second one.

pkg/util/grpcutil/fast_metadata.go line 93 at r1 (raw file):

			newMd[k] = v
		}
		return found, val, context.WithValue(ctx, grpcIncomingKeyObj, newMd)

Is this context.WithValue() right? It looks to me like grpc wants a rawMD value, not a metadata.MD.
Why are we not using the vanillaNewOutgoingContext()?

pkg/util/grpcutil/fast_metadata.go line 132 at r1 (raw file):

type fakeContext struct {
	grpcIncomingKeyObj interface{}

Maybe call this recordedKey. Took me a second to get what's happening in this acid trip.

pkg/util/grpcutil/fast_metadata.go line 137 at r1 (raw file):

var _ context.Context = (*fakeContext)(nil)

func (f *fakeContext) Value(keyObj interface{}) interface{} {

lolo you gotta put a comment on this hack

pkg/util/grpcutil/fast_metadata.go line 31 at r2 (raw file):

//   - The caller promises to not modify the returned MD -- the gRPC
//     APIs assume that the map in the context remains constant.
func FastFromIncomingContext(ctx context.Context) (metadata.MD, bool) {

wanna use this in grpc_interceptor.go?

pkg/util/grpcutil/fast_metadata.go line 74 at r2 (raw file):

// from the given metadata key, if it exists. If it does, the metadata
// key is removed from the context.
func FastGetAndDeleteValueFromIncomingContext(

I'd say let's wait to introduce this one when it's actually used. I don't know if you can actually get this to work - see the comment below.

But beyond than that, the more I look around, the more I think that we should not be blindly forwarding random metadata. For the tracing metadata, I think it's rather a bad thing that we propagate it - when A calls B call C, we don't want C's span to be tied to A's span; I think that do more harm than good. In practice B will override the span ID (although it gets real funky in some edge cases). I also looked at the use of metadata in the blob service. That one is a pretty ridiculous use of metadata, but I'm sure it doesn't care about any forwarding. And generally, I think the next unsuspecting user of metadata will also not expect or want this magic propagation (which is only done for some RPCs, btw).
So I really think we should switch to propagating the user name explicitly. I can't imagine anything falling out of it.

knz

RFAL

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @benbardin)

pkg/util/grpcutil/fast_metadata.go line 40 at r1 (raw file):