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

Cancelled status code not reported in the gRPC server metrics #5109

Closed
ceremo opened this issue May 17, 2024 · 6 comments · Fixed by #5137
Closed

Cancelled status code not reported in the gRPC server metrics #5109

ceremo opened this issue May 17, 2024 · 6 comments · Fixed by #5137
Labels
enhancement A general enhancement
Milestone

Comments

@ceremo
Copy link

ceremo commented May 17, 2024

Hi,

cancelled status codes are not reported in the gRPC server metrics, metrics in these cases are reported without the tag status code because GrpcServerObservationContext is not updated with the cancelled. For us it's important the gRPC server metric give us info of the status codes, and this could be improved for example modifying the onCancel method of the ObservationGrpcServerCallListener before running the stop operation, something like this:

  @Override
  public void onCancel() {
    try (final Scope scope = this.observation.openScope()) {
      super.onCancel();
    } finally {
      if (this.observation.getContext() instanceof final GrpcServerObservationContext obsCtx) {
        obsCtx.setStatusCode(Status.Code.CANCELLED);
      }
      this.observation.stop();
    }
  }

I have created a demo to reproduce easily cancelleds, the service has a delay of 5 seconds to give time to the client to cancel the request, you could make the call using postman or a simple grpc client with a deadline lower than 5 seconds.

Thanks for your time.

@ttddyy
Copy link
Contributor

ttddyy commented May 17, 2024

Observing the behavior of cancel, it seems the status code doesn't change even if the request is canceled.
Instead, the ServiceCall#isCancelled returns true for canceled requests.

So, I'm inclined to add GrpcServerObservationContext#isCancelled, which becomes a low cardinality tag, to indicate whether the request has been canceled or not, then keep the existing status code unchanged.

@ceremo
Copy link
Author

ceremo commented May 20, 2024

It works for me, cancellations in webflux are defined using connectionAborted.

ttddyy added a commit to ttddyy/micrometer that referenced this issue May 20, 2024
ttddyy added a commit to ttddyy/micrometer that referenced this issue May 20, 2024
ttddyy added a commit to ttddyy/micrometer that referenced this issue May 22, 2024
ttddyy added a commit to ttddyy/micrometer that referenced this issue May 22, 2024
@jonatan-ivanov jonatan-ivanov added enhancement A general enhancement and removed waiting-for-triage labels May 22, 2024
@jonatan-ivanov jonatan-ivanov added this to the 1.14.0-M1 milestone May 22, 2024
ttddyy added a commit to ttddyy/micrometer that referenced this issue May 22, 2024
@JordiMartinezVicent
Copy link

Hi @marcingrzejszczak @ttddyy

With the soluction provided, it creates a new event with status cancelled but it could be useful to add the cancel information to the GrpcServerObservationContext which is used to retrieve the timer metric.

The same behavior is resolved in webflux adding a boolean at ServerRequestObservationContext

@jonatan-ivanov
Copy link
Member

You mean you simply want this info added to the context even if it is not used here so that you can create an ObservationFilter or an ObservationHandler and use it?

Or do you want the instrumentation also add a low cardinality key-value about the fact of the cancellation (that can also mean removing the event)?

Either case, could you create a new issue and explain what you want and how you want to use this information?

I think right now you can hack this: it's not great but you can create an ObservationHandler that receives the cancellation event (onEvent) method and reacts to that (e.g.: sets a field/keyvalue on the context, etc).

@JordiMartinezVicent
Copy link

JordiMartinezVicent commented Jul 12, 2024

You mean you simply want this info added to the context even if it is not used here so that you can create an ObservationFilter or an ObservationHandler and use it?

Or do you want the instrumentation also add a low cardinality key-value about the fact of the cancellation (that can also mean removing the event)?

I think that both would be helpful. On the one hand, we want to differentiate that the timer metric generated has been cancelled. And on the other hand, we have a custom GrpcServerObservationConvention to add extra information, which would be helpful to know if the request has been cancelled.

And yes, at least in our case, the event would have no sense.

Either case, could you create a new issue and explain what you want and how you want to use this information?

Perfect!

I think right now you can hack this: it's not great but you can create an ObservationHandler that receives the cancellation event (onEvent) method and reacts to that (e.g.: sets a field/keyvalue on the context, etc).

I will explore it, thanks for the suggestion!

@JordiMartinezVicent
Copy link

@jonatan-ivanov I have just created the following issue #5301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants