orca: allow a ServerMetricsProvider to be passed to the ORCA service and ServerOption #6223
Conversation
dfawley
added
the
Type: API Change
orca/call_metrics.go
Outdated
| type callMetricsRecorderCtxKey struct{} | ||
|
|
||
| // CallMetricsRecorderFromContext returns the RPC specific custom metrics | ||
| // recorder [CallMetricsRecorder] embedded in the provided RPC context. |
There was a problem hiding this comment.
The text inside [] does not appear as links in the godoc. Are we missing something?
There was a problem hiding this comment.
Hmm, IDK, I just renamed this file and made minor edits for the new names. I think you wrote the docstring originally. 😛
Deleted.
orca/call_metrics.go
Outdated
|
|
||
| b, err := proto.Marshal(sm.toLoadReportProto()) | ||
| if err != nil { | ||
| logger.Warningf("failed to marshal load report: %v", err) |
There was a problem hiding this comment.
Nit: s/failed/Failed/, here and two lines below.
| } | ||
| return service, nil | ||
| } | ||
|
|
||
| // Register creates a new ORCA service implementation configured using the | ||
| // provided options and registers the same on the provided service registrar. | ||
| func Register(s *grpc.Server, opts ServiceOptions) (*Service, error) { | ||
| func Register(s *grpc.Server, opts ServiceOptions) error { |
There was a problem hiding this comment.
I see that this is existing code, but should/could we accept a grpc.ServiceRegistrar instead? If not, maybe we should fix the docstring.
There was a problem hiding this comment.
orca/server_metrics.go
Outdated
| func (s *serverMetricsRecorder) ServerMetrics() *ServerMetrics { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| ret := &ServerMetrics{ |
There was a problem hiding this comment.
Nit: return the value directly from this line.
orca/call_metrics.go
Outdated
| type callMetricsRecorderCtxKey struct{} | ||
|
|
||
| // CallMetricsRecorderFromContext returns the RPC specific custom metrics | ||
| // recorder [CallMetricsRecorder] embedded in the provided RPC context. |
There was a problem hiding this comment.
Hmm, IDK, I just renamed this file and made minor edits for the new names. I think you wrote the docstring originally. 😛
Deleted.
orca/call_metrics.go
Outdated
|
|
||
| b, err := proto.Marshal(sm.toLoadReportProto()) | ||
| if err != nil { | ||
| logger.Warningf("failed to marshal load report: %v", err) |
orca/server_metrics.go
Outdated
| func (s *serverMetricsRecorder) ServerMetrics() *ServerMetrics { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| ret := &ServerMetrics{ |
| } | ||
| return service, nil | ||
| } | ||
|
|
||
| // Register creates a new ORCA service implementation configured using the | ||
| // provided options and registers the same on the provided service registrar. | ||
| func Register(s *grpc.Server, opts ServiceOptions) (*Service, error) { | ||
| func Register(s *grpc.Server, opts ServiceOptions) error { |
There was a problem hiding this comment.
…nd per-call ServerOption
|
LGTM |
…and ServerOption (grpc#6223)
This is a pretty significant amount of churn but no major new functionality.
...Metric...to...Metrics...in the names of things.call_metric_recorder.gotocall_metrics.go(and_testequivalent).orca.goto new fileserver_metrics.go.Utilizationin setter toNamedUtilizationto match C++.QPS,EPS, andNamedMetricsfields and setters/getters.ServerMetricsprovider & Server/Call recorders.ServerMetricsProviderforNewService/RegisterandCallMetricsServerOption.Note that there are several API changes here, but this package is still experimental so it is allowable.
This brings us up to spec for the updates in gRFC A51, and is needed for A58.
RELEASE NOTES: