Hotchocolate: DiagnosticSource logic clears existing diagnostic listeners

[tunurgitr]

Is your feature request related to a problem? Please describe.
When using HotChocolate in combination with other DiagnosticSource listeners (I've tested with Application Insights, OpenTracing, and Elasticsearch APM), those listeners get unregistered by HotChocolate's logic here that clears out any existing DiagnosticSource or DiagnosticListener implementation: https://github.com/ChilliCream/hotchocolate/blob/master/src/Core/Core/Execution/Extensions/QueryExecutionBuilderExtensions.cs#L169-L171.

I came across this trying to understand why I couldn't visualize delegated queries in my projects using hotchocolate. What I realized is that the DiagnosticListener for AppInsights/OpenTracing/ElasticAPM wasn't able to listen for the incoming web request, since its listeners had registered themselves with a different instance of the DiagnisticSource (I think that's what's happening).

Describe the solution you'd like
I'd like to see a conventional diagnostic source implementation similar to what MS does for ado.net. That way users can subscribe to events as described here: https://sudonull.com/posts/19303-Using-the-DiagnosticSource-in-NET-Core-Theory.

Ideally that would be accompanied by a something similar to what we have now where there's a strongly-typed wrapper class or something to avoid users having to write their own boilerplate code to pull data out of key value pairs.

That could look similar to what exists now so that users could keep their same basic approach.

[freerkjelsma]

We are also running into this issue, request tracking stops working for ApplicationInsights as soon as the DiagnosticListener is registered in UseInstrumentation.

[tunurgitr]

@michaelstaib does this release resolve this issue? https://github.com/ChilliCream/hotchocolate/releases/tag/10.4.1-preview.1

[freerkjelsma]

I've tested earlier with a custom pipeline and it seems to me that it stops working as soon as the DiagnosticListener instance is added as singleton:

builder.Services.AddSingleton(listener)

I've tested on 11.0.0-preview.118

[Athosone]

Hi guys,

We are currently refactoring our services to use graphql with hotchocolate however we faced the same issue so its on pause until this issue is resolved.

Any ideas when will it be fixed ?

Regards

[tunurgitr]

It’s fixed in the latest 10.4.1 release

On Thu, Apr 9, 2020 at 6:59 AM WERCK Ayrton [e-mail]
wrote:

Hi guys,

We are currently refactoring our services to use graphql with hotchocolate
however we faced the same issue so its on pause until this issue is
resolved.

Any ideas when will it be fixed ?

Regards

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ChilliCream/hotchocolate/issues/1227#issuecomment-611543519,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAMIDXIQUHUGJSFPZ4JYEBDRLXIFTANCNFSM4JR5GLTQ
.

[mikolajszczepanski]

Hi guys,

We are using 10.4.2 of hotchocolate and we are having issues with logging requests using Microsoft.ApplicationInsights.AspNetCore.

I debugged ApplicationInsights and his listener doesn't receive messages emitted after hotchocolate is registered in DI through https://github.com/ChilliCream/hotchocolate/blob/10.4.2/src/Core/Core/Execution/Extensions/QueryExecutionBuilderExtensions.cs#L173,L174 so these following events are not processed any more: https://github.com/aspnet/Hosting/blob/master/src/Microsoft.AspNetCore.Hosting/Internal/HostingApplicationDiagnostics.cs (like "Microsoft.AspNetCore.Hosting.HttpRequestIn.Start") and because of it requests are not logged into Application Insights.

Is any workaround for this?

Kind Regards

[otryshko]

https://hotchocolategraphql.slack.com/archives/CD9TNKT8T/p1587153848069300?thread_ts=1584691561.196700

If anyone is looking for a solution while it is being solved in 11.0, here is an ugly hack

1. build service provider, resolve and save DiagnosticListener/DiagnosticSource instances
2. Run services.AddGraphQL
3. Remove DiagnosticListener/DiagnosticSource from services
4. Add DiagnosticListener/DiagnosticSource saved at step 1.
5. Enjoy your get requests in AI (and probably now is something broken in HC diagnostics).

[Athosone]

Tested with the latest version and still broken .... 10.4.3

[michaelstaib]

This one is now fixed with version 11.

[ogliznutsa]

@michaelstaib do you have plans to backport the fix to v10 as v11 is still in preview?

[michaelstaib]

We will do no more fixed for 10 since 11 will be out soon.