search

spring-projects-experimental / spring-grpc

88 stars 19 forks source link

#1: Autoconfigure metric registry support #41

Closed hmtang closed 2 weeks ago

hmtang commented 3 weeks ago

Signed-off-by: Sunny hmtang@gmail.com [resolves #1]

hmtang commented 3 weeks ago

Thanks a lot for the feedbacks. I would be out for a short trip in next week. I would look into this when I get back. Appreciate your time on the reviews.

making commented 3 weeks ago

I don't think we need MetricCollectingServerInterceptor if we support ObservationGrpcServerInterceptor which can output similar metrics and traces at the same time.

onobc commented 3 weeks ago

I don't think we need MetricCollectingServerInterceptor if we support ObservationGrpcServerInterceptor which can output similar metrics and traces at the same time.

I agree w/ this @making . I think we should just add auto-configuration for the Micrometer Observations API (which gives metrics and tracing) and if users want to only use Micrometer metrics they can add it in via interceptor themselves.

We have started taking this approach in our newer Spring projects (e.g. Spring Pulsar only provides support for Observations API). Most Spring projects were incepted prior to the availability of Observations API therefore Micrometer support was already in place.

@dsyer do you agree w/ this?

dsyer commented 3 weeks ago

100%

hmtang commented 2 weeks ago

Tried to change to include only AutoConfiguration for the Metric, I am not entire sure about whether this is enough for the change. Please have a look. Thanks a lot.

hmtang commented 2 weeks ago

Thanks for the reviews. I have pushed a new changes for the requested changes.

dsyer commented 2 weeks ago

"This branch cannot be rebased safely" according to github, so you need to rebase. Probably a good idea to squash your commits into one as well and force push back to your fork.

onobc commented 2 weeks ago

Thanks again for the great contribution @hmtang . I am closing this PR in favor of https://github.com/spring-projects-experimental/spring-grpc/commit/7fed50a42008713703c9004da12b6880eb6223c3.

I added a follow-on commit to your commit (see details in commit message if you are interested. I add a couple more tests).

onobc commented 2 weeks ago

"This branch cannot be rebased safely" according to github, so you need to rebase. Probably a good idea to squash your commits into one as well and force push back to your fork.

Sorry @dsyer , I went ahead and handled that locally when I added polish commit. Next time I will assign the PR to myself to give a signal.