- Notifications
You must be signed in to change notification settings - Fork4.5k
orca: allow a ServerMetricsProvider to be passed to the ORCA service and ServerOption#6223
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM.
Sorry for the delay. It took a long time to warm up to the new API, with the confusion between call metrics and server metrics.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The text inside[] does not appear as links in the godoc. Are we missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: s/failed/Failed/, here and two lines below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
| } | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see that this is existing code, but should/could we accept agrpc.ServiceRegistrar instead? If not, maybe we should fix the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: return the value directly from this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Comments addressed; thanks!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
| } | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
…nd per-call ServerOption
LGTM |
Uh oh!
There was an error while loading.Please reload this page.
This is a pretty significant amount of churn but nomajor 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 theupdates ingRFC A51, and is needed forA58.
RELEASE NOTES: