[bug] Memory leak with long-running bidirectional streams

[Falco20019]

Component

OpenTelemetry.Instrumentation.GrpcCore

Package Version

Package Name	Version
OpenTelemetry.Api	1.9.0
OpenTelemetry	1.9.0
OpenTelemetry.Exporter.OpenTelemetryProtocol	1.9.0
OpenTelemetry.Extensions.Hosting	1.9.0
OpenTelemetry.Instrumentation.GrpcCore	1.0.0-beta.6

Runtime Version

net8.0

Description

The ServerTracingInterceptor+ServerRpcScope<TRequest, TResponse> is having a reference to an Activity. For long-running calls like a bi-directional stream for ADS on xDS this means that DiagNode<T> elements will be gathered and kept in memory for the WHOLE lifetime of the application per gRPC channel. In our case, it generated 5 GB of RAM before we had to restart the application.

In case of long-running activities, Microsoft advices to instead use short-lived sub-activities instead and setting subActivity.SetParentId(parentActivity.TraceId, parentActivity.SpanId); to link them (to avoid multi-threading possibly leading to faulty parents). We had to do the same fix on some of our libraries for the same reason.

Steps to Reproduce

1. Create a long-running connection
2. Spam thousands of gRPC calls
3. Check the RAM usage

Expected Result

No memory leak and activities not being held indefinetly

Actual Result

Memory leak due to RpcScope keeping one scope for the whole call's lifecycle without creating child-activities.

Additional Context

We sadly are bound to OpenTelemetry 1.9.0 since 1.10.0 switched Microsoft.Extensions.Diagnostics.Abstractions and Microsoft.Extensions.Logging.Configuration to 9.x as min-versions. According to semantic versioning this would have been a breaking change and should have been reflected by using 2.0.0 instead. Since we follow semver2 and can't break our upstream users, we therefore have to stick with 8.x on net8.0 and only started using the newer versions on net9.0.

See open-telemetry/opentelemetry-dotnet#5967 (comment)

[github-actions]

Tagging component owner(s).

@IliaBrahinets @pcwiese

[pcwiese]

Aside from the memory leak due to super long running rpcs, any possibility to switch to the grpc dotnet instrumentation instead of of grpc.core based one? Can you recycle the channel periodically? It could be a while before I can look into this...

[Falco20019]

Sadly not as we use MAUI and since ASP.NET is still not supported there, switching is not possible. Also cycling is not possible since it's done by gRPC's c-core itself, so nothing I can influence by any means.

dotnet/aspnetcore#35077 is sadly still unresolved for 4 years.

I will have a look and maybe I'll be able to contribute a PR.

[IliaBrahinets]

I'm going to take a look at it next week. @Falco20019, as far as I understood you have ServerTracingInterceptorOptions.RecordMessageEvents enabled, right ?

[Falco20019]

Correct, have it enabled.

[Falco20019]

@IliaBrahinets I also have another somewhat related issue/limitation: open-telemetry/opentelemetry-dotnet#6228

Maybe you have a hint about how it should be done correctly.

[IliaBrahinets]

The current implementation follows the specification correctly:
"In the lifetime of an RPC stream, an event for each message sent/received on client and server spans SHOULD be created. In case of unary calls only one sent and one received message will be recorded for both client and server spans." - OT spec
So, this is rather a flaw in the design of RPC message handling and not .NET specific (e.g. open-telemetry/opentelemetry-go-contrib#436). There are plans to deprecate Span Event API and most probably RPC conventions will be re-designed - open-telemetry/opentelemetry-specification#4430, open-telemetry/semantic-conventions#2038.

Similar issues which don't provide any clear answer on the matter:

• Could Activity.Events cause memory exhaustion in long running activities? dotnet/runtime#42347
• Record gRPC message events opentelemetry-dotnet#1276

Currently you can disable ServerTracingInterceptorOptions.RecordMessageEvents and ClientTracingInterceptorOptions.RecordMessageEvents or the instrumentation completely and instrument RPC messages on the application level.

Within dotnet/dotnet-api-docs#4871 it's been explicitly stated in the docs that callers of Activity.AddEvent should either restrict the number of events or use another mechanism such as logging. We could add a configurable limit for the number of events submitted from the instrumentation. @cijothomas, @Kielek what do you think ?

Thanks to @Kielek and @lmolkova for consultation on the matter.

[Falco20019]

The problem is less the amount of events but more that the Activity is kept open for a very long time and never finished until the application stops in the worst case. This also means that spans will not end up in the tracing due to that, making long-running streams untraceable. This is why we usually have short-lived sub-activities added per-event on the streaming calls (basically all non-unary ones) for other tracing occasions.

Disabling ServerTracingInterceptorOptions.RecordMessageEvents and ClientTracingInterceptorOptions.RecordMessageEvents will reduce the memory needed but still not resolve the problem that no data ends up on the tracing. But yeah, @JamesNK already saw it coming 5 years ago :(

[Falco20019]

@IliaBrahinets Any updates on this?