Make statistics thread safe

[nscuro]

Feature Request

I have a use case where I want to expose persistence metrics via Micrometer to give users more insight into how their system behaves. To achieve this, I have enabled DataNucleus' statistics feature via datanucleus.enablestatistics=true, and registered the collected FactoryStatistics with Micrometer. For example:

FunctionCounter.builder(METRICS_PREFIX + "datastore_reads_total", pmf,
                        p -> p.getNucleusContext().getStatistics().getNumberOfDatastoreReads())
                .description("Total number of read operations from the datastore")
                .register(Metrics.getRegistry());

Gauge.builder(METRICS_PREFIX + "queries_active", pmf,
                        p -> p.getNucleusContext().getStatistics().getQueryActiveTotalCount())
                .description("Number of currently active queries")
                .register(Metrics.getRegistry());

In my case, binding to individual ManagerStatistics is not practical due to PersistenceManager instances being too short-lived. I was hoping that FactoryStatistics would provide me with a reliable aggregate, but the numbers I'm seeing made me realize that the way statistics work today is not thread safe.

For reference, on my system with lots of concurrent tasks, I was regularly seeing DN reporting negative quantities of active queries, or astronomically high connection counts that would never decline.

To my understanding, ManagerStatistics are associated with an ExecutionContext, which itself is associated with a PersistenceManager. Each ManagerStatistics instance has a parent, referring to a FactoryStatistics instance.

This means that FactoryStatistics is modified by multiple threads concurrently. However, it doesn't have any synchronization logic that would accommodate for this.

My feature request is to make at least FactoryStatistics thread safe. Usage of AtomicInteger over plain int for most counters may already solve a large chunk of this problem.

[nscuro]

Just to provide a suggestion and get the discussion going, I raised #478.

[chrisco484]

Is there any affect on performance if stats are not enabled?

[nscuro]

@chrisco484 AFAICT all interactions with statistics objects are guarded with if conditions, so presumably no.

[chrisco484]

@chrisco484 AFAICT all interactions with statistics objects are guarded with if conditions, so presumably no.

That probably would have been the only objection I can think of: if that 'int' was suddenly to become 'AtomicInteger' it could introduce a synchronized resource (even though the blocking time would be tiny) assuming AtomicInteger has some implicit synchronizing/blocking deep in any JVM.

As you say its access is guarded by if conditions so I agree we wouldn't expect any performance impact.

[nscuro]

Closing because #478 has been merged.