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

add new highcardmetrics pkg to grpcmetrics #171

Merged
merged 8 commits into from
Feb 11, 2022

Conversation

agnaite
Copy link
Member

@agnaite agnaite commented Feb 9, 2022

Context

We want to add two new options to the grpc server, which allows us to collect metrics using attributes.

The new highcardmetrics pkg provides high cardinality unary and stream server interceptors. The interceptors take an explicit bool arg, which makes the histogram type configurable. Passing in true will create explicit histograms with a default 5s distribution.

Usage

To initialize a grpc server with high cardinality unary and stream servers using explicit histograms:

gSrv, hSrv := grpcserver.NewStandardH2C(
  apiSrv,
  // high cardinality interceptors using explicit histograms
  grpcserver.HighCardInterceptors(
     highcardmetrics.NewUnaryServerInterceptor(o.metricsProvider, true), 
     highcardmetrics.NewStreamServerInterceptor(o.metricsProvider, true),
   ),
)

Copy link
Member

@voidlock voidlock left a comment

Choose a reason for hiding this comment

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

Yup, this is exactly what I was thinking.


service, method := parseFullMethod(info.FullMethod)

labels := []string{"service", service, "method", method}
Copy link
Member

Choose a reason for hiding this comment

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

Will this service conflict (or be confused) with the existing service* columns?

Copy link
Member Author

Choose a reason for hiding this comment

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

very true. how do you feel about grpc.service and grpc.method? Or any other suggestions welcome!

Copy link
Member

Choose a reason for hiding this comment

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

It's like you can read my mind, I love those choices!

In fact, I was wondering about changing ours in the httpmetrics middleware to use http.method, http.path, http.response-status.

Comment on lines 160 to 162
func ms(d time.Duration) float64 {
return float64(d) / float64(time.Millisecond)
}
Copy link
Member

Choose a reason for hiding this comment

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

There is a much easier way to do this now https://pkg.go.dev/time#Duration.Milliseconds

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch! just a copy/paste from the other code.


defer func(begin time.Time) {
service, method := parseFullMethod(info.FullMethod)
labels := []string{"service", service, "method", method, "response-status", code(err)}
Copy link
Member

Choose a reason for hiding this comment

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

These labels will need to get tweaked as well.

@agnaite agnaite self-assigned this Feb 11, 2022
@agnaite agnaite marked this pull request as ready for review February 11, 2022 19:50
Copy link
Member

@voidlock voidlock left a comment

Choose a reason for hiding this comment

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

Looks great!

@agnaite agnaite merged commit 2116165 into master Feb 11, 2022
@agnaite agnaite deleted the add-high-card-grpc-metrics branch February 11, 2022 22:01
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