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

binarylog: Account for key in metadata truncation #5851

Merged
merged 2 commits into from
Dec 9, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions gcp/observability/logging_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ func cmpLoggingEntryList(got []*grpcLogEntry, want []*grpcLogEntry) error {
if len(a) > len(b) {
a, b = b, a
}
if len(a) == 0 && len(a) != len(b) { // No metadata for one and the other comparator wants metadata.
return false
}
for k, v := range a {
if b[k] != v {
return false
Expand Down Expand Up @@ -1145,3 +1148,126 @@ func (s) TestMarshalJSON(t *testing.T) {
t.Fatalf("json.Marshal(%v) failed with error: %v", logEntry, err)
}
}

// TestMetadataTruncationAccountsKey tests that the metadata truncation takes
// into account both the key and value of metadata. It configures an
// observability system with a maximum byte length for metadata, which is
// greater than just the byte length of the metadata value but less than the
// byte length of the metadata key + metadata value. Thus, in the ClientHeader
// logging event, no metadata should be logged.
func (s) TestMetadataTruncationAccountsKey(t *testing.T) {
fle := &fakeLoggingExporter{
t: t,
}
defer func(ne func(ctx context.Context, config *config) (loggingExporter, error)) {
newLoggingExporter = ne
}(newLoggingExporter)

newLoggingExporter = func(ctx context.Context, config *config) (loggingExporter, error) {
return fle, nil
}
configMetadataLimit := &config{
ProjectID: "fake",
CloudLogging: &cloudLogging{
ClientRPCEvents: []clientRPCEvents{
{
Methods: []string{"*"},
MaxMetadataBytes: 6,
Copy link
Member

Choose a reason for hiding this comment

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

This would be slightly nicer as:

const mdValue = "value"
...
MaxMetadataBytes: len(mdValue) + 1,
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice suggestion. Switched.

},
},
},
}

cleanup, err := setupObservabilitySystemWithConfig(configMetadataLimit)
if err != nil {
t.Fatalf("error setting up observability %v", err)
}
defer cleanup()

ss := &stubserver.StubServer{
UnaryCallF: func(ctx context.Context, in *grpc_testing.SimpleRequest) (*grpc_testing.SimpleResponse, error) {
return &grpc_testing.SimpleResponse{}, nil
},
}
if err := ss.Start(nil); err != nil {
t.Fatalf("Error starting endpoint server: %v", err)
}
defer ss.Stop()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()

// the set config MaxMetdataBytes is in between len("value") and len("key")
// + len("value"), and thus shouldn't log this metadata entry.
md := metadata.MD{
"key": []string{"value"},
}
ctx = metadata.NewOutgoingContext(ctx, md)
if _, err := ss.Client.UnaryCall(ctx, &grpc_testing.SimpleRequest{Payload: &grpc_testing.Payload{Body: []byte("00000")}}); err != nil {
t.Fatalf("Unexpected error from UnaryCall: %v", err)
}

grpcLogEntriesWant := []*grpcLogEntry{
{
Type: eventTypeClientHeader,
Logger: loggerClient,
ServiceName: "grpc.testing.TestService",
MethodName: "UnaryCall",
Authority: ss.Address,
SequenceID: 1,
Payload: payload{
Metadata: map[string]string{},
},
PayloadTruncated: true,
},
{
Type: eventTypeClientMessage,
Logger: loggerClient,
ServiceName: "grpc.testing.TestService",
MethodName: "UnaryCall",
SequenceID: 2,
Authority: ss.Address,
Payload: payload{
MessageLength: 9,
Message: []uint8{},
},
PayloadTruncated: true,
},
{
Type: eventTypeServerHeader,
Logger: loggerClient,
ServiceName: "grpc.testing.TestService",
MethodName: "UnaryCall",
SequenceID: 3,
Authority: ss.Address,
Payload: payload{
Metadata: map[string]string{},
},
},
{
Type: eventTypeServerMessage,
Logger: loggerClient,
ServiceName: "grpc.testing.TestService",
MethodName: "UnaryCall",
Authority: ss.Address,
SequenceID: 4,
},
{
Type: eventTypeServerTrailer,
Logger: loggerClient,
ServiceName: "grpc.testing.TestService",
MethodName: "UnaryCall",
SequenceID: 5,
Authority: ss.Address,
Payload: payload{
Metadata: map[string]string{},
},
},
}
fle.mu.Lock()
if err := cmpLoggingEntryList(fle.entries, grpcLogEntriesWant); err != nil {
fle.mu.Unlock()
t.Fatalf("error in logging entry list comparison %v", err)
}
fle.mu.Unlock()
}
2 changes: 1 addition & 1 deletion internal/binarylog/method_logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func (ml *TruncatingMethodLogger) truncateMetadata(mdPb *pb.Metadata) (truncated
// but not counted towards the size limit.
continue
}
currentEntryLen := uint64(len(entry.Value))
currentEntryLen := uint64(len(entry.GetKey())) + uint64(len(entry.GetValue()))
if currentEntryLen > bytesLimit {
break
}
Expand Down