dgs-framework icon indicating copy to clipboard operation
dgs-framework copied to clipboard

bug: Getting tracing data for all the requests

Open AyushChaubey opened this issue 3 years ago • 3 comments

This issue is an extension of the original issue which I had raised. The documentation was updated to suggest what needs to be done to enable tracing in a federated setup.

The issue now is that we are getting the tracing information for all the requests. Even though the tracing information is not requested from Apollo Gateway. Even if you run the GraphQL server standalone, it will reply back with the ftv1 header in the response with the tracing information. The issue is happening because one of the type check is failing in the federated JVM library. Please refer the code here . Here, the context is of type DGSContext and not of type HTTPRequestHeaders. This type check fails and the shouldTrace function returns true by default. This results in getting the trace for all the requests. In general, we request tracing only for a fraction of the requests to avoid the computation load.

Expected behavior

We should be only getting the tracing information for the requests in which tracing header is passed from the Gateway.

Actual behavior

We are getting tracing information for all the requests.

Steps to reproduce

This can be easily reproduced in any GraphQL service implemented using the suggested tracing mechanism here.

AyushChaubey avatar May 09 '22 12:05 AyushChaubey

Hi. Can we please get an update on this. It will help us in again enabling the tracing.

AyushChaubey avatar Jun 08 '22 08:06 AyushChaubey

Fixing that bug would be absolutely amazing!

aabdelhafez avatar Sep 26 '22 13:09 aabdelhafez

Thanks for reporting! We are not able to get to this anytime soon due to internal priorities. We do welcome PRs, so feel free to contribute any fixes to the issue if this is blocking. We can add that to the upcoming release in that case. Thanks again!

srinivasankavitha avatar Sep 26 '22 16:09 srinivasankavitha

I believe that this will work:

@Component
public class ApolloContextForwarder implements GraphQLContextContributor {
        @Override
        public void contribute(@NotNull GraphQLContext.Builder builder, @Nullable Map<String, ?> extensions, @Nullable DgsRequestData dgsRequestData) {
            if (dgsRequestData == null || dgsRequestData.getHeaders() == null) {
                return;
            }

            final HttpHeaders headers = dgsRequestData.getHeaders();

            // if the header exists, we should just forward it
            if (headers.containsKey(FederatedTracingInstrumentation.FEDERATED_TRACING_HEADER_NAME)) {
                builder.put(
                        FederatedTracingInstrumentation.FEDERATED_TRACING_HEADER_NAME,
                        headers
                                .get(FederatedTracingInstrumentation.FEDERATED_TRACING_HEADER_NAME)
                                .stream()
                                .findFirst()
                                .get()
                );
            }  else {
                //otherwise, place a value != "ftv1" so when it gets checked for == ftv1 it fails
                // and trace does not happen
                builder.put(FederatedTracingInstrumentation.FEDERATED_TRACING_HEADER_NAME, "DO_NOT_TRACE");
            }
        }
}

You can toss this anywhere in your project and it will only trace if the headers are present and the correct value (ftv1).

It feels odd to me that the default behavior is to trace if the header is not found - feels like the default should be not to trace. I hesitate to make a PR for this because it would have to be specific logic to Apollo gateway federated tracing, since it's behavior is strange.

antholeole avatar Oct 21 '22 15:10 antholeole

Hey @antholeole, thx a lot! I can confirm that this workaround is also working for us!

hpuac avatar Nov 14 '22 19:11 hpuac

@antholeole - thanks so much for posting this. And you're right in that we would like to avoid adding logic specific to integrating in the apollo gateway as much as possible. It would be good to add this to our docs in any case for folks looking to integrate with Apollo's tracing. Here is our docs repo, if you're interested in contributing: https://github.com/Netflix/dgs

Else, I'll get to it in a bit.

srinivasankavitha avatar Nov 14 '22 23:11 srinivasankavitha

I’ll add this to the docs!

antholeole avatar Nov 15 '22 01:11 antholeole