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 "cancelled" information to the GrpcServerObservationContext #5301

Closed
JordiMartinezVicent opened this issue Jul 12, 2024 · 3 comments
Closed
Labels
enhancement A general enhancement instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Milestone

Comments

@JordiMartinezVicent
Copy link

JordiMartinezVicent commented Jul 12, 2024

Derived from the comment #5109 (comment)

When creating a Grpc Server metric, we would like to know either the request has been cancelled or not. Currently an observation event is generated, but this information is lost for the Grpc Server timer metric.

It would be nice the both approaches which @jonatan-ivanov at the comment:

  • Add the information to the context so we can use it at custom ObservationFilter, ObservationHandler or GrpcServerObservationConvention
  • Add the information as a low cardinality key-value so we can differentiate which resquests have been cancelled.

Something similar to this commit would be nice: ttddyy@53313ef but I do not know why it was discarded.

@shakuzen
Copy link
Member

Thank you for opening the issue and giving feedback on the approach taken to resolve #5109.

Adding the info to the context sounds like an easy thing to do with little concern for effects on other things. Whether to add a low cardinality key-value I think warrants some additional discussion to understand the need and what's missing currently.

Something similar to this commit would be nice: ttddyy@53313ef but I do not know why it was discarded.

There was some offline discussion about which approach to take (event vs tag). It sounds like you have a valuable perspective that would have contributed to the discussion. In general, tags that have a boolean value (or rather some string representation of a boolean value) are a bit of a smell, in my opinion. It would be ideal to encode such information in an otherwise meaningful tag such as the status code tag. However, I see from #5109 (comment) we couldn't go that direction because there is a status code irrespective of the cancelled status. I lack experience with grpc but I see from https://grpc.github.io/grpc/core/md_doc_statuscodes.html that there is a CANCELLED status, and the second table indicates it should be used on both the client and server side. Can someone help me understand the original issue why this isn't happening with our instrumentation? /cc @ttddyy

@JordiMartinezVicent
Copy link
Author

Adding the info to the context sounds like an easy thing to do with little concern for effects on other things.

Yes! It would be nice having that information independently of whether or not having the "cancelled" information as low cardinality key-value.

I do not know why grpc does not set the status as CANCELLED, in my opinion it would have sense. Having said that, we internally change the status in order to have this information in our metrics. Having the info at the context, would have our logic quite more easy.

@shakuzen
Copy link
Member

shakuzen commented Aug 9, 2024

@ttddyy would you happen to have any spare time to look at this?

ttddyy added a commit to ttddyy/micrometer that referenced this issue Sep 4, 2024
Add gRPC cancellation information to the `GrpcServerObservationContext`.
Also, improve gRPC unit test reliability by adding proper wait mechanisms.

See micrometer-metricsgh-5301

Signed-off-by: Tadaya Tsuyukubo <tadaya@ttddyy.net>
@shakuzen shakuzen added enhancement A general enhancement module: micrometer-core An issue that is related to our core module instrumentation An issue that is related to instrumenting a component labels Sep 4, 2024
@shakuzen shakuzen added this to the 1.14.0-M3 milestone Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

No branches or pull requests

2 participants