ObservationGrpcServerCallListener does not capture an uncontrolled error during the execution of the service logic

[ceremo]

The ObservationGrpcServerCallListener currently does not inform to the Observation object when an uncontrolled error occurs during the execution of the gRPC service logic. The current behavior does not allow to determine if an exception ocurred, so the generated metric cannot report accurately if the gRPC service failed.

In our gRPC services, errors can be generated as signals (next, completed, error...), but they can also occur as uncontrolled errors during service execution. What are your thoughts about this?

[ceremo]

@ttddyy

[marcingrzejszczak]

Do you have an example that replicates this problem so that we can look into it?

[github-actions]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[ceremo]

No, but you only have to check the behaviour of the method onHalfClose defined in ObservationGrpcServerCallListener, when the grpc server fails with an unexpected error, the exception is captured but it does not pass the error to the observation:

  public void onHalfClose() {
        try (Scope scope = observation.openScope()) {
            super.onHalfClose();
        }
    }

Thxs for your time.

[marcingrzejszczak]

@ttddyy so we would have to do this change and it will capture the exception?

From

public void onHalfClose() {
        try (Scope scope = observation.openScope()) {
            super.onHalfClose();
        }
    }

To

public void onHalfClose() {
        observation.scoped( () -> super.onHalfClose() );
    }

[ttddyy]

Sorry for the late reply.
Yes, wrapping with scoped sounds good to me.

[marcingrzejszczak]

@ceremo are you willing to this change? It's pretty straight-forward :)

[ceremo]

Thanks @marcingrzejszczak. I would like your opinion in two improvements about the behaviour of the grpc observation:

• ObservationGrpcServerInterceptor does not allow to customize the Supplier<GrpcServerObservationContext>, this could be useful in order to add extra information to the context.
• DefaultGrpcServerObservationConvention only receives the metadata of the request, it would be useful to get the metadata of the response on the close execution, to do that the ObservationGrpcServerCall should set in the context the response metadata, like the status:

  @Override
  public void close(final Status status, final Metadata trailers) {
    if (status.getCause() != null) {
      this.observation.error(status.getCause());
    }

    final GrpcServerObservationContext context = (GrpcServerObservationContext) this.observation.getContext();
    context.setStatusCode(status.getCode());
    context.setCarrier(trailers); // HERE THE IMPROVEMENT
    super.close(status, trailers);
  }

Thanks for your time.

[marcingrzejszczak]

ObservationGrpcServerInterceptor does not allow to customize the Supplier, this could be useful in order to add extra information to the context.

The ObservationConvention mechanism should be used to add extra information.

DefaultGrpcServerObservationConvention only receives the metadata of the request, it would be useful to get the metadata of the response on the close execution, to do that the ObservationGrpcServerCall should set in the context the response metadata, like the status:

We already are doing this.

[ceremo]

The ObservationConvention mechanism should be used to add extra information.

Sorry, I don't get it, the ObservationConvention already receives the context fulfilled, and it is the grpc interceptor that is fullfilling it.

[marcingrzejszczak]

You said that you want to add extra information to the context. What information exactly? Are you saying that you're missing some objects that the convention could later reuse ? Cause I understood that assuming that all the information got already provided you wanted to add more tags on the context in which case the ObservationConvention would be the way to go.

[ceremo]

We need to include additional information to the GrpcServerObservationContext in order to convert it into additional tags in the generated metric. That extra information is included in the Metadata, so to include that in the context I have to define my own ObservationGrpcServerInterceptor because this does not allow customizing the contextSupplier. Finally the ObservationConvention load that additional information to include it as new tags.

What I'm trying to say it that allowing to customize the supplier context defined in the interceptor will facilitate our case, and could be useful to have it.

[marcingrzejszczak]

It would be better IMO to prepare the context accordingly. If you think we're missing things in the context please file a PR that adds this data to the existing context.