Could Activity.Events cause memory exhaustion in long running activities?

[JamesNK]

Activity.Events is new in .NET 5. Is there any concern that a long running activity, with many events, could create a memory leak cause memory exhaustion in an app?

The specific scenario I'm thinking of is a long running gRPC streaming call. There is an activity on the client and server that lasts for the duration of the call. In OpenTelemetry each sent and received message is an event. See https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/rpc.md#events. Message activities would be added to the call activity as events.

If a call sends and receives hundreds of thousands of messages over the lifetime of a call, and each time adds a new ActivityEvent + attributes to the current activity, I believe those events will stay in-memory until the call is complete. Memory usage over the lifetime of a long running call will continuously increase.

Is my theory correct? What is the guidance here?

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

[JamesNK]

Reason why I am thinking about this - open-telemetry/opentelemetry-dotnet#1276

[tarekgh]

CC @noahfalk

It depends on your scenario. If you want to capture all events during the gRPC streaming call then you have to pay the memory price for that. And yes the allocated memory expected to grow but there will not be any leak as soon the Activity stopped and GC collected, all allocated memory will be freed again. This is not different then AddTag for instance.

If the memory is a concern in your scenario, you can control how many events you want add and drop the rest (not adding them). Similar idea as sampling.

[noahfalk]

Hi @JamesNK thanks for raising the issue!
Poking around I see java uses a bounded queue whereas .NET currently has an unbounded one. I was searching in the specification and I may have missed it but I didn't see any indication of how OT is expecting implementations to uniformly handle large numbers of events. .NET is currently assuming the producer won't create overly large numbers of events vs. the java impl which is treating data size as the SDK's problem to manage.

In the short term I don't think we have time to change the behavior for .NET 5 even if we wanted to. Instead we'd have to decide what do we think is the best grpc behavior knowing how the Activity impl currently behaves and should we be trying to make a change in a future release?
Short term the main solutions I could see would be:

• At some point grpc stops adding new events so a single Activity only has a record of the first N events
• Store the events in a separate bounded queue while the Activity is progress, then copy out of the queue into the Activity prior to completing it. This gives, I assume, more Java-like behavior where the last N events are captured (as opposed to the first N events)
• Any other arbitrarily complex sampling mechanism that picks N of the events to store in the Activity

I don't see a strong reason that the last N would be preferable to the first N or arbitrary N so I'd be tempted to do the simplest and highest perf option, the first one IMO.

For the longer term solution it wouldn't be hard technically to implement limits in Activity but it would be a breaking change in behavior which is fairly worrisome. If grpc is the sole scenario or one of relatively few scenarios that would ever emit high volumes of events it might be easier and safer to let grpc enforce a limit rather than enforce it within Activity itself. I'd be curious to hear from @cijothomas and @reyang if the OpenTelemetry community is looking at future improvements for how large event volumes are handled. The current design suggests that diagnosing issues in these high message count grpc streams wasn't a priority for OT and the only bar they are trying to meet is that high event volume shouldn't cause the process to fail with an OOM.

I don't know how satisfying that is, but hope it helps somewhat and happy to get your thoughts James.

[JamesNK]

And yes the allocated memory expected to grow but there will not be any leak as soon the Activity stopped and GC collected, all allocated memory will be freed again. This is not different then AddTag for instance.

Lets call it an easy way to cause memory exhaustion instead.

The difference between tags and events is there are a finite number of tags an app would want to add, while events can be unbounded.

If grpc is the sole scenario or one of relatively few scenarios that would ever emit high volumes of events it might be easier and safer to let grpc enforce a limit rather than enforce it within Activity itself.

I don't know how common it would be. As a server dev I'm paranoid about accidentally killing a server by using up resources, so I think about this sort of thing, but it is an easy mistake for anyone using Activity/Events to make. There is nothing about activities that naturally limits its duration, and nothing about events today that limits the count.

I can definitely work around this in gRPC, a configurable limit on message events is easy enough, but I worry for other users of this API.

[blowdart]

Is there enough documentation on the Activity class to warn of such an occurance? If not, warnings need to put in docs now.

And the implicit support for sampling, or an upper bound later, because otherwise I can see this being an easy way to write a component that DoSes a web site or service (although the bug there would be in whatever is using the Activity classes, not the underlying class itself, but we would want a pit of sucess to make it easier to use safely)

[tarekgh]

@blowdart we can add something to the docs.

For DoSes I am not sure I understand this. how this is different if the component allocate a low of memory even without using Activity at all.

[blowdart]

It's about setting expectations and making sure we're clear, so when someone says it's a security problem I can point to the docs :)

And it's not different from allocating an endless dictionary, but as you can see from James's use and the workarounds you have to talk about it's just not as obvious as a dictionary (plus memory caches have limits, you don't have any yet, so it just needs calling out)