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.

Sign up for GitHub

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

orca: minor cleanups #6239

Merged
merged 2 commits into from
May 2, 2023
Merged

orca: minor cleanups #6239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f642735
orca: minor cleanups
dfawley Apr 26, 2023
c287632
comments & tests
dfawley May 1, 2023
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
17 changes: 7 additions & 10 deletions orca/call_metric_recorder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/orca"
"google.golang.org/grpc/orca/internal"

v3orcapb "github.com/cncf/xds/go/xds/data/orca/v3"
testgrpc "google.golang.org/grpc/interop/grpc_testing"
Expand All @@ -58,7 +59,6 @@ func (s) TestE2ECallMetricsUnary(t *testing.T) {
desc string
injectMetrics bool
wantProto *v3orcapb.OrcaLoadReport
wantErr error
}{
{
desc: "with custom backend metrics",
Expand All @@ -73,7 +73,6 @@ func (s) TestE2ECallMetricsUnary(t *testing.T) {
{
desc: "with no custom backend metrics",
injectMetrics: false,
wantErr: orca.ErrLoadReportMissing,
},
}

Expand Down Expand Up @@ -146,9 +145,9 @@ func (s) TestE2ECallMetricsUnary(t *testing.T) {
t.Fatalf("EmptyCall failed: %v", err)
}

gotProto, err := orca.ToLoadReport(trailer)
if test.wantErr != nil && !errors.Is(err, test.wantErr) {
t.Fatalf("When retrieving load report, got error: %v, want: %v", err, orca.ErrLoadReportMissing)
gotProto, err := internal.ToLoadReport(trailer)
if err != nil {
t.Fatalf("When retrieving load report, got error: %v, want: <nil>", err)
}
if test.wantProto != nil && !cmp.Equal(gotProto, test.wantProto, cmp.Comparer(proto.Equal)) {
t.Fatalf("Received load report in trailer: %s, want: %s", pretty.ToJSON(gotProto), pretty.ToJSON(test.wantProto))
Expand All @@ -165,7 +164,6 @@ func (s) TestE2ECallMetricsStreaming(t *testing.T) {
desc string
injectMetrics bool
wantProto *v3orcapb.OrcaLoadReport
wantErr error
}{
{
desc: "with custom backend metrics",
Expand All @@ -180,7 +178,6 @@ func (s) TestE2ECallMetricsStreaming(t *testing.T) {
{
desc: "with no custom backend metrics",
injectMetrics: false,
wantErr: orca.ErrLoadReportMissing,
},
}

Expand Down Expand Up @@ -288,9 +285,9 @@ func (s) TestE2ECallMetricsStreaming(t *testing.T) {
}
}

gotProto, err := orca.ToLoadReport(stream.Trailer())
if test.wantErr != nil && !errors.Is(err, test.wantErr) {
t.Fatalf("When retrieving load report, got error: %v, want: %v", err, orca.ErrLoadReportMissing)
gotProto, err := internal.ToLoadReport(stream.Trailer())
if err != nil {
t.Fatalf("When retrieving load report, got error: %v, want: <nil>", err)
}
if test.wantProto != nil && !cmp.Equal(gotProto, test.wantProto, cmp.Comparer(proto.Equal)) {
t.Fatalf("Received load report in trailer: %s, want: %s", pretty.ToJSON(gotProto), pretty.ToJSON(test.wantProto))
Expand Down
39 changes: 38 additions & 1 deletion orca/internal/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,16 @@
// avoid polluting the godoc of the top-level orca package.
package internal

import ibackoff "google.golang.org/grpc/internal/backoff"
import (
"errors"
"fmt"

ibackoff "google.golang.org/grpc/internal/backoff"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I saw Easwar comment something about import renaming with no alias/conflict and you replied with to keep it consistent with other packages that conflict with backoff. Does this apply to this?

Copy link
Member Author

Choose a reason for hiding this comment

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

backoff is a top-level package so I wouldn't want to use that name. It looks like we use the more verbose "internalbackoff" in other places. This usage is existing so I'd rather just leave it alone and avoid churn.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I always get confused with balancer/ vs. internal/balancer conflicts. Maybe we should start adopting conventions for internal packages prefixed with i.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be fine with something like that...having consistent names is always good.

"google.golang.org/grpc/metadata"
"google.golang.org/protobuf/proto"

v3orcapb "github.com/cncf/xds/go/xds/data/orca/v3"
)

// AllowAnyMinReportingInterval prevents clamping of the MinReportingInterval
// configured via ServiceOptions, to a minimum of 30s.
Expand All @@ -32,3 +41,31 @@ var AllowAnyMinReportingInterval interface{} // func(*ServiceOptions)
//
// For testing purposes only.
var DefaultBackoffFunc = ibackoff.DefaultExponential.Backoff

// TrailerMetadataKey is the key in which the per-call backend metrics are
// transmitted.
const TrailerMetadataKey = "endpoint-load-metrics-bin"

// ToLoadReport unmarshals a binary encoded [ORCA LoadReport] protobuf message
// from md and returns the corresponding struct. The load report is expected to
// be stored as the value for key "endpoint-load-metrics-bin".
//
// If no load report was found in the provided metadata, if multiple load
// reports are found, or if the load report found cannot be parsed, an error is
// returned.
//
// [ORCA LoadReport]: (https://github.com/cncf/xds/blob/main/xds/data/orca/v3/orca_load_report.proto#L15)
func ToLoadReport(md metadata.MD) (*v3orcapb.OrcaLoadReport, error) {
vs := md.Get(TrailerMetadataKey)
if len(vs) == 0 {
return nil, nil
}
if len(vs) != 1 {
return nil, errors.New("multiple orca load reports found in provided metadata")
zasweq marked this conversation as resolved.
Show resolved Hide resolved
}
ret := new(v3orcapb.OrcaLoadReport)
if err := proto.Unmarshal([]byte(vs[0]), ret); err != nil {
return nil, fmt.Errorf("failed to unmarshal load report found in metadata: %v", err)
zasweq marked this conversation as resolved.
Show resolved Hide resolved
}
return ret, nil
}
35 changes: 8 additions & 27 deletions orca/orca.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,19 @@ package orca
import (
"context"
"errors"
"fmt"

"google.golang.org/grpc"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/internal"
igrpc "google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/balancerload"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/orca/internal"
"google.golang.org/protobuf/proto"

v3orcapb "github.com/cncf/xds/go/xds/data/orca/v3"
)

var (
logger = grpclog.Component("orca-backend-metrics")
joinServerOptions = internal.JoinServerOptions.(func(...grpc.ServerOption) grpc.ServerOption)
joinServerOptions = igrpc.JoinServerOptions.(func(...grpc.ServerOption) grpc.ServerOption)
)

const trailerMetadataKey = "endpoint-load-metrics-bin"
Expand Down Expand Up @@ -144,26 +142,6 @@ func (w *wrappedStream) Context() context.Context {
// ErrLoadReportMissing indicates no ORCA load report was found in trailers.
var ErrLoadReportMissing = errors.New("orca load report missing in provided metadata")

// ToLoadReport unmarshals a binary encoded [ORCA LoadReport] protobuf message
// from md and returns the corresponding struct. The load report is expected to
// be stored as the value for key "endpoint-load-metrics-bin".
//
// If no load report was found in the provided metadata, ErrLoadReportMissing is
// returned.
//
// [ORCA LoadReport]: (https://github.com/cncf/xds/blob/main/xds/data/orca/v3/orca_load_report.proto#L15)
func ToLoadReport(md metadata.MD) (*v3orcapb.OrcaLoadReport, error) {
vs := md.Get(trailerMetadataKey)
if len(vs) == 0 {
return nil, ErrLoadReportMissing
}
ret := new(v3orcapb.OrcaLoadReport)
if err := proto.Unmarshal([]byte(vs[0]), ret); err != nil {
return nil, fmt.Errorf("failed to unmarshal load report found in metadata: %v", err)
}
return ret, nil
}

// loadParser implements the Parser interface defined in `internal/balancerload`
// package. This interface is used by the client stream to parse load reports
// sent by the server in trailer metadata. The parsed loads are then sent to
Expand All @@ -174,9 +152,12 @@ func ToLoadReport(md metadata.MD) (*v3orcapb.OrcaLoadReport, error) {
type loadParser struct{}

func (loadParser) Parse(md metadata.MD) interface{} {
lr, err := ToLoadReport(md)
lr, err := internal.ToLoadReport(md)
if err != nil {
logger.Errorf("Parse(%v) failed: %v", err)
logger.Infof("Parse failed: %v", err)
}
if lr == nil && logger.V(2) {
logger.Infof("Missing ORCA load report data")
}
return lr
}
Expand Down
34 changes: 19 additions & 15 deletions orca/orca_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,18 @@ import (
"github.com/google/go-cmp/cmp"
"google.golang.org/grpc/internal/pretty"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/orca"
"google.golang.org/grpc/orca/internal"

v3orcapb "github.com/cncf/xds/go/xds/data/orca/v3"
)

func TestToLoadReport(t *testing.T) {
goodReport := &v3orcapb.OrcaLoadReport{
CpuUtilization: 1.0,
MemUtilization: 50.0,
RequestCost: map[string]float64{"queryCost": 25.0},
Utilization: map[string]float64{"queueSize": 75.0},
}
tests := []struct {
name string
md metadata.MD
Expand All @@ -40,7 +46,7 @@ func TestToLoadReport(t *testing.T) {
{
name: "no load report in metadata",
md: metadata.MD{},
wantErr: true,
wantErr: false,
},
{
zasweq marked this conversation as resolved.
Show resolved Hide resolved
name: "badly marshaled load report",
Expand All @@ -49,29 +55,27 @@ func TestToLoadReport(t *testing.T) {
}(),
wantErr: true,
},
{
name: "multiple load reports",
md: func() metadata.MD {
b, _ := proto.Marshal(goodReport)
return metadata.Pairs("endpoint-load-metrics-bin", string(b), "endpoint-load-metrics-bin", string(b))
}(),
wantErr: true,
},
{
name: "good load report",
md: func() metadata.MD {
b, _ := proto.Marshal(&v3orcapb.OrcaLoadReport{
CpuUtilization: 1.0,
MemUtilization: 50.0,
RequestCost: map[string]float64{"queryCost": 25.0},
Utilization: map[string]float64{"queueSize": 75.0},
})
b, _ := proto.Marshal(goodReport)
return metadata.Pairs("endpoint-load-metrics-bin", string(b))
}(),
want: &v3orcapb.OrcaLoadReport{
CpuUtilization: 1.0,
MemUtilization: 50.0,
RequestCost: map[string]float64{"queryCost": 25.0},
Utilization: map[string]float64{"queueSize": 75.0},
},
want: goodReport,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got, err := orca.ToLoadReport(test.md)
got, err := internal.ToLoadReport(test.md)
if (err != nil) != test.wantErr {
t.Fatalf("orca.ToLoadReport(%v) = %v, wantErr: %v", test.md, err, test.wantErr)
}
Expand Down
2 changes: 1 addition & 1 deletion orca/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func (s *Service) determineReportingInterval(req *v3orcaservicepb.OrcaLoadReport
}
dur := req.GetReportInterval().AsDuration()
if dur < s.minReportingInterval {
logger.Warningf("Received reporting interval %q is less than configured minimum: %v. Using default: %s", dur, s.minReportingInterval)
logger.Warningf("Received reporting interval %q is less than configured minimum: %v. Using minimum", dur, s.minReportingInterval)
return s.minReportingInterval
}
return dur
Expand Down