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

Server metrics are tagged with the wrong status codes #789

Open
harti2006 opened this issue Nov 24, 2022 · 13 comments · May be fixed by #1134
Open

Server metrics are tagged with the wrong status codes #789

harti2006 opened this issue Nov 24, 2022 · 13 comments · May be fixed by #1134
Labels
bug Something does not work as expected feedback required Information are missing or feedback for suggestions is requested

Comments

@harti2006
Copy link

The context

I'd like to get the metric values for grpc.server.processing.duration and distinguish different status codes, e.g. OK, NOT_FOUND, INVALID_ARGUMENT, UNKNOWN

The bug

If my gRPC service is called three times, and it responds twice with an OK status and once with with an NOT_FOUND status, I'd expect the tags that are captured for the metrics grpc.server.processing.duration to be "OK" and "NOT_FOUND".
But instead, the "NOT_FOUND" error seems to be reported as an "UNKNOWN" error.

Stacktrace and logs

I don't know

Steps to Reproduce

I created a very simple example project, with one gRPC service and one GrpcAdvice that translates my "business" exception into a NOT_FOUND status.

Please check out the integration test, that shows how to reproduce the issue: https://github.com/harti2006/grpc-metrics-demo/blob/trunk/src/test/java/com/github/harti2006/grpc/metrics/GrpcMetricsDemoApplicationTests.java

The application's environment

Which versions do you use?

  • Spring (boot): 2.7.5
  • grpc-java: 1.51.0
  • grpc-spring-boot-starter: 2.14.0.RELEASE
  • java: 11

Additional context

  • It used to work fine with the previous release 2.13.1.RELEASE. When downgrading to this version, the above mentioned integration test works fine
@harti2006 harti2006 added the bug Something does not work as expected label Nov 24, 2022
@ST-DDT
Copy link
Collaborator

ST-DDT commented Nov 24, 2022

Which version of micrometer are you using? Because we removed our implementation if favor of the official/upstream one.

@ST-DDT ST-DDT added the feedback required Information are missing or feedback for suggestions is requested label Nov 24, 2022
@harti2006
Copy link
Author

harti2006 commented Nov 24, 2022 via email

@ST-DDT
Copy link
Collaborator

ST-DDT commented Nov 24, 2022

Have a look here: I assume these should be the locations you are looking for:

@harti2006
Copy link
Author

Yeah, I found these places while debugging the problem. And I saw that funny behavior: The responseCode is only updated in the close method, but that method was only called when the grpc handler method completed successfully. On errors, the close method wasn't called at all, so the default value "UNKNOWN" was reported.
Tomorrow, I will be able to add another case to my example to verify if this happens only when using the GrpcAdvice approach of sending errors, or if it's a general problem.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Nov 24, 2022

@ST-DDT
Copy link
Collaborator

ST-DDT commented Nov 25, 2022

😭 The current grpc advice implementation seems to be broken and might have to be reimplemented.
For unary calls it seems to work to a certain extend, but causes an undefined state in the call itself.
For unary calls it doesn't work (anymore) as it tries to close the call twice, and logs errors.

@harti2006
Copy link
Author

Thanks for investigating. As promised I added another case to the example project where I send the exception directly via streamObserver:

responseObserver.onError(Status.NOT_FOUND.withDescription("goodbye message not found").asException());

I did the same test as before and with this approach, the metrics were reported correctly. See https://github.com/harti2006/grpc-metrics-demo/blob/933c29641a3c813db649cdd275ba022e08637820/src/test/java/com/github/harti2006/grpc/metrics/GrpcMetricsDemoApplicationTests.java#L72

I also tried your suggestion with changing the order of the MetricCollectingServerInterceptor bean, but it made things even worse: No metrics were reported at all.

@pawelKapl
Copy link

Hey, i have the very same issue. Did u managed to fix that problem? I am afraid i will need to downgrade this lib to 2.13.1. Do u need any help in investigation?

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jul 17, 2023

A up/downgrade of this library should not have an impact on that as the official metric implementation is the exact same as this one's AFAIK, and I PRed it there.

@pawelKapl
Copy link

Hmm OP mentioned that downgrade to 2.13.1 is resolving this issue. What do u suggest then?

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jul 17, 2023

I think it doesnt hurt trying.
If it does solve the problem, it would be nice if you could debug into it and compare what is different.

@pawelKapl
Copy link

pawelKapl commented Jul 18, 2023

@ST-DDT going back to 2.13.1 helped (SpringBoot 2.7.12 here). In a free time will take a look at both versions and try to find issue there.

@joaomper-TE
Copy link

Any way of using @GrpcAdvice with Spring Boot 3.X? responseObserver.onError( is not working for us.

@s4kas s4kas linked a pull request Aug 15, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as expected feedback required Information are missing or feedback for suggestions is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants