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

gRPC server metrics #14922

Merged
merged 7 commits into from Oct 11, 2022
Merged

gRPC server metrics #14922

merged 7 commits into from Oct 11, 2022

Conversation

pglass
Copy link

@pglass pglass commented Oct 7, 2022

Description

This updates the internal and external gRPC servers with the following labeled metrics, where the server_type label is either internal or external.

Server metrics:

  • grpc.server.connection.count{server_type=internal|external}
  • grpc.server.connections{server_type=internal|external}
  • grpc.server.request.count{server_type=internal|external}
  • grpc.server.stream.count{server_type=internal|external}
  • grpc.server.streams{server_type=internal|external}

Client metrics (a client only exists for the internal gRPC server)

  • grpc.client.connection.count{server_type=internal}
  • grpc.client.connections{server_type=internal}
  • grpc.client.request.count{server_type=internal}

These are the same metrics that currently exist for the internal gRPC server, except those are now emitted with a server_type = internal label.

The stats handler is moved to the agent/grpc-middleware package, and some shared unit test stuff is moved to the agent/grpc-middleware/testutil package.

Testing & Reproduction steps

  • Unit tests

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

"github.com/hashicorp/consul/proto/prototest"
)

func TestServer_EmitsStats(t *testing.T) {
Copy link
Author

@pglass pglass Oct 7, 2022

Choose a reason for hiding this comment

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

This copies the TestHandler_EmitStats test. I did some deduplication of the helpers by moving things over to grpc-middleware/testutil. (I could do more deduplication but kind of wanted a sanity check before I go too far.)

Copy link
Contributor

@boxofrad boxofrad left a comment

Choose a reason for hiding this comment

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

Nice work! 🚢

)

func TestServer_EmitsStats(t *testing.T) {
sink, reset := patchGlobalMetrics(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of passing the metrics object as a parameter to NewServer so we don't have to change global state like this?

func NewServer(logger agentmiddleware.Logger, metrics *metrics.Metrics) *grpc.Server {
	if metrics == nil {
		metrics = agentmiddleware.DefaultMetrics
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

I also appreciate that these tests were copied from grpc-internal so please feel free to leave as-is! 🙇🏻‍♂️

Copy link
Author

Choose a reason for hiding this comment

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

💯 I removed the global metrics instance from both grpc-internal and grpc-external

@pglass pglass merged commit d17af23 into main Oct 11, 2022
@pglass pglass deleted the pglass/grpc-xds-metrics branch October 11, 2022 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants