Add flexibility for using GraphQL

[glucaci]

Hi,

We are working on a prototype to integrate into APM a GraphQL request using HotChocolate.AspNetCore.

For this to look more like GraphQL and not REST we would like to replace the AspNetCoreDiagnosticListener completely because we want to control the Result of the main transaction. (e.g. to don't have as result HTTP2xx or HTTP5xx but Success or Failed).

At the moment this is not possible because the Result is set on HttpRequestIn.Stop event.

I would suggest:

1. Split AspNetCoreDiagnosticListener into:

• AspNetCoreHttpDiagnosticListener which hast the Http stack events HttpRequestIn.Start and HttpRequestIn.Stop
• AspNetCoreHostingDiagnosticListener which has the rest of events which are useful in any case,

2. Make TraceContext or maybe WebRequestTransactionCreator public, to can use the useful implementation of parsing and creating/filling transaction.

... or to have a way to hook in StopTransaction and provide some custom Result and Outcome

I will be glad to make a PR for this.

Thanks!

[gregkalapos]

Hi,

the idea of some sorts of hook into StopTransaction came up a few times, but so far we always ended up not doing it.

Is this only about the Result property?

If so, you could also create a filter similar to the one below which will translate from HTTP results to Success/Failed.

Agent.AddFilter((ITransaction transaction) =>
{
	transaction.Result =
		transaction.Result.Equals("HTTP2xx", StringComparison.OrdinalIgnoreCase)
			? "Success"
			: "Failed";
	return transaction;
});

[glucaci]

It's also about Outcome and every thing based on HTTP Status Codes.

Because GraphQL has always a HTTP 200, base on response payload we can decide if was a Success, Partial Failed or Failed response. So it's not only replacing the HTTP2xx with Success or Failed.

1. It will be very helpful some AfterStop hook.

2. Or what do you thing about the other ideea to split AspNetCoreDiagnosticListener and to make some helper classes public?

3. Or maybe something directly on transaction

ITransaction transaction;
transaction.BeforeEnd((ITransaction t) => { } );

then we could setup the hook when the graphql request ended and we have the result

But i think the most flexibility also for other cases will bring the point 2)

[gregkalapos]

It's also about Outcome and every thing based on HTTP Status Codes.

I see - you have access to basically all fields in a filter, you can also set Outcome and read other fields. But thinking more about it, this indeed won't help you.

Or what do you thing about the other ideea to split AspNetCoreDiagnosticListener and to make some helper classes public?

I'm not sure this would be the key here. First of all, this will complicate other scenarios - you can manually decide which listeners you want to turn on, and if we split it, people need to know they need to turn on multiple ones for ASP.NET Core.

First I'd like to explore if we could extend it and make it aware of GrapQL. We did the same with gRPC.

There are more things here to properly support GraphQL. We already have a spec for it. As you can see, the transaction name should also be handled by it.

I'm looking at this - I think having a listener specifically listening for those events would be helpful - that gives us more info and we don't need to parse the body.

Let me look into this in more detail...

[glucaci]

There are more things here to properly support GraphQL. We already have a spec for it. As you can see, the transaction name should also be handled by it.

I will take a look into it.

I'm not sure this would be the key here. First of all, this will complicate other scenarios - you can manually decide which listeners you want to turn on, and if we split it, people need to know they need to turn on multiple ones for ASP.NET Core.

Or maybe we can have an option to opt-out, something like DisableAspNetCorePipeline and then everything stays together and is enabled by default.

I'm looking at this - I think having a listener specifically listening for those events would be helpful - that gives us more info and we don't need to parse the body.

This is outdated, it was in v10, but in v11 it was removed because of performance, as I know (but for sure @michaelstaib can help us with some infos). For example on a query with 100 fields you will had at least 100 events for every filed resolver,

In v11 the client must explicitly register in GraphQL Server a IDiagnosticEvents

services
    .AddGraphQLServer()
    .AddDiagnosticEventListener(sp => new ElasticApmDiagnosticListener())

Of course we could write a diagnostic listener which is generating DiagnosticSource only for specific actions.

This is what i've done till now:

1. with AspNetCoreDiagnosticListener

2. without AspNetCoreDiagnosticListener where the transaction is not complete because is not propagating the traceId

My main concern was that I want to be able to filter transaction by success or failure from the Transactions main filter and not by some custom query.

That's why the idea came to bring the spited transactions together with the code that is already implemented in elastic agent.

I will share my sample repo where you can test also who is working.

[glucaci]

I can understand that maybe splitting has more consequences, but I can say with the actual implementation we could do a nice transaction letting apart the Result.

We have a sample with 5 services : 4 domains and 1 gateway (stitching)

query myReviews { # operation name
  me() { # gateway delegation to service accounts, query user
    id  
    name
    birthdate
    reviews { # gateway delegation to service reviews, query reviewsByAuthor
      id
      product { # gateway delegation to service products, query product
        name
        price
        inStock # gateway delegation to service inventory, query inventoryInfo
        shippingEstimate # gateway delegation to service inventory, query shippingEstimate
      }
    }
  }
}  

The problem is that you cannot build something generic in the agent because every framework has it's own implementation. What we could do is to define a list of events needed to build the entire trace and each framework will build an adapter for that.

The way that we approached was that for each execution we have [OperationName].[RootField] and to batch execution we are creating also a span for each field. For sure we have to test with some bigger queries to don't end up with big transactions.

[glucaci]

Hi @gregkalapos do you have any thoughts on this. Maybe will be good to kind of define a generic API on agent level and every graphql framework will write an adapter for it.

[glucaci]

Hi @gregkalapos, As you specified above I've created the possibility to overwrite the Result and Outcome in the same way as Grpc is implemented.

If this is good to go we can close this issue.

[glucaci]

I've close the PR because using Activity it's not possible. Any Activity created on a lower level that HttpRequestIn will not survive back on the line because of the way how AsyncLocal is working.

With Grpc is another story because it's getting created on a higher level than HttpRequestIn.

My only idea with the current design how we can hook on the Transaction and change the Result and Outcome is to bring the EventHandler Ended on ITransaction. Otherwise we have to introduce another concept for 3rd party libraries to can do this.

@russcam @gregkalapos what are your thoughts about this? We really need a way to hook this up, otherwise we don't have relevant information in APM.

[glucaci]

@russcam @gregkalapos any news on this?

[glucaci]

@russcam do you have any thoughts on this? I can help with implementation if we can define how it should look.