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

Missing option to get response headers in GrpcServerObservationConvention #4012

Closed
rafal-dudek opened this issue Aug 10, 2023 · 2 comments · Fixed by #4516
Closed

Missing option to get response headers in GrpcServerObservationConvention #4012

rafal-dudek opened this issue Aug 10, 2023 · 2 comments · Fixed by #4516
Labels
enhancement A general enhancement
Milestone

Comments

@rafal-dudek
Copy link
Contributor

Please describe the feature request.
Object of class GrpcServerObservationContext is missing response headers, making it impossible to create some KeyValues based on headers values in custom GrpcServerObservationConvention.

Class: https://github.com/micrometer-metrics/micrometer/blob/main/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/grpc/ObservationGrpcServerCall.java should override public void sendHeaders(Metadata headers) method and add response headers to GrpcServerObservationContext .

Only available headers are from the request e.g. context.getGetter().get(context.getCarrier(), "content-type").

Response headers could be added by context.put(Metadata.class, headers) but separate field would be the best option, e.g.

class ObservationGrpcServerCall<ReqT, RespT> extends ForwardingServerCall.SimpleForwardingServerCall<ReqT, RespT> {
...
    @Override
  public void sendHeaders(Metadata headers) {
    super.sendHeaders(headers);
    GrpcServerObservationContext context = (GrpcServerObservationContext)this.observation.getContext();
    context.setResponseHeaders(headers);
  }
}

Rationale
We want to add some metrics/tracing tags depending on headers values.
This option is available for Http e.g:

public class CustomServerRequestObservationConvention extends DefaultServerRequestObservationConvention {
    public static final String ERROR_NAME = "/error/name";

    @Override
    public KeyValues getHighCardinalityKeyValues(ServerRequestObservationContext context) {
        return super.getHighCardinalityKeyValues(context)
                .and(errorName(context));
    }

    protected KeyValue errorName(ServerRequestObservationContext context) {
        if (context.getResponse() != null) {
            return KeyValue.of(ERROR_NAME, context.getResponse().getHeader("error-type"));
        }
        return KeyValue.of(ERROR_NAME, KeyValue.NONE_VALUE);
    }
}

Additional context
This option was also available for Tracing in Sleuth library e.g.:

        @Bean
        public RpcTracingCustomizer rpcTracingCustomizer() {
            return rpcTracing -> {
                rpcTracing.serverResponseParser(new CustomRpcResponseParser());
            };
        }

        public static class CustomRpcResponseParser extends RpcResponseParser.Default {

            @Override
            public void parse(RpcResponse response, TraceContext context, SpanCustomizer span) {
                super.parse(response, context, span);
                if (response instanceof GrpcServerResponse) {
                    Metadata metadata = ((GrpcServerResponse) response).headers();
                    if (metadata.containsKey(HeadersConstants.getMetadataKey("error-type"))) {
                        span.tag("/error/name", metadata.get(HeadersConstants.getMetadataKey("error-type")));
                    }
                }
            }
        }
@marcingrzejszczak
Copy link
Contributor

WDYT @ttddyy ? For me it seems like a reasonable request.

@ttddyy
Copy link
Contributor

ttddyy commented Dec 30, 2023

looking at the gRPC API, the javadoc of ServerCall#sendHeaders method says:

Since Metadata is not thread-safe, the caller must not access (read or write) headers after this point.

So, we should not directly store the passed headers(Metadata) object in context.
Looking at the Brave implementation, there is an easy way to do it.

I'll take a look.

FYI, the RpcResponseParser/GrpcServerResponse in the "Additonal context" snippet are from Brave.
So, it should work as long as using Brave and setting them up correctly.

ttddyy added a commit to ttddyy/micrometer that referenced this issue Dec 31, 2023
Resolves micrometer-metrics#4012

Signed-off-by: Tadaya Tsuyukubo <tadaya@ttddyy.net>
marcingrzejszczak pushed a commit that referenced this issue Jan 2, 2024
Resolves #4012

Signed-off-by: Tadaya Tsuyukubo <tadaya@ttddyy.net>
@marcingrzejszczak marcingrzejszczak added this to the 1.13.0-M1 milestone Jan 2, 2024
pirgeo pushed a commit to dynatrace-oss-contrib/micrometer that referenced this issue Jan 5, 2024
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.

3 participants