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

orca: allow a ServerMetricsProvider to be passed to the ORCA service and ServerOption #6223

Merged
merged 2 commits into from May 2, 2023

Conversation

dfawley
Copy link
Contributor

@dfawley dfawley commented Apr 25, 2023

This is a pretty significant amount of churn but no major new functionality.

  1. Rename ...Metric... to ...Metrics... in the names of things.
  2. Rename call_metric_recorder.go to call_metrics.go (and _test equivalent).
  3. Move parts of orca.go to new file server_metrics.go.
  4. Rename Utilization in setter to NamedUtilization to match C++.
  5. Add QPS, EPS, and NamedMetrics fields and setters/getters.
  6. Split Server/Call metrics into one ServerMetrics provider & Server/Call recorders.
  7. Accept a ServerMetricsProvider for NewService/Register and CallMetricsServerOption.
  8. Update examples accordingly.

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:

  • orca: allow a ServerMetricsProvider to be passed to the ORCA service and ServerOption

@dfawley dfawley added the Type: API Change Breaking API changes (experimental APIs only!) label Apr 25, 2023
@dfawley dfawley added this to the 1.56 Release milestone Apr 25, 2023
@dfawley dfawley requested a review from easwars April 25, 2023 21:27
@dfawley dfawley mentioned this pull request May 1, 2023
Copy link
Contributor

@easwars easwars left a 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.

type callMetricsRecorderCtxKey struct{}

// CallMetricsRecorderFromContext returns the RPC specific custom metrics
// recorder [CallMetricsRecorder] embedded in the provided RPC context.
Copy link
Contributor

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?

Copy link
Contributor Author

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.


b, err := proto.Marshal(sm.toLoadReportProto())
if err != nil {
logger.Warningf("failed to marshal load report: %v", err)
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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 a grpc.ServiceRegistrar instead? If not, maybe we should fix the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We needed the ORCA protos to be regenerated for this:

cncf/xds#41

Updated the comment.

func (s *serverMetricsRecorder) ServerMetrics() *ServerMetrics {
s.mu.Lock()
defer s.mu.Unlock()
ret := &ServerMetrics{
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@easwars easwars assigned dfawley and unassigned easwars May 2, 2023
Copy link
Contributor Author

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Comments addressed; thanks!

type callMetricsRecorderCtxKey struct{}

// CallMetricsRecorderFromContext returns the RPC specific custom metrics
// recorder [CallMetricsRecorder] embedded in the provided RPC context.
Copy link
Contributor Author

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.


b, err := proto.Marshal(sm.toLoadReportProto())
if err != nil {
logger.Warningf("failed to marshal load report: %v", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

func (s *serverMetricsRecorder) ServerMetrics() *ServerMetrics {
s.mu.Lock()
defer s.mu.Unlock()
ret := &ServerMetrics{
Copy link
Contributor Author

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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We needed the ORCA protos to be regenerated for this:

cncf/xds#41

Updated the comment.

@easwars
Copy link
Contributor

easwars commented May 2, 2023

LGTM

@dfawley dfawley merged commit add9015 into grpc:master May 2, 2023
11 checks passed
@dfawley dfawley deleted the orca branch May 2, 2023 22:04
tobotg pushed a commit to tobotg/grpc-go that referenced this pull request May 3, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: API Change Breaking API changes (experimental APIs only!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants