[PECOBLR-1408] [PECOBLR-1407] [PECOBLR-1409][PECOBLR-1411] Address telemetry code audit comments- part1

[samikshya-db]

Description

• 4 things that is improved with respect to telemetry :
  • Common object mapper across telemetry use-case (This is already thread safe and is expensive to create, i.e., good tor re-use)
  • MakeflushIntervalMillis config same across both telemetry clients (un-auth and auth)
  • Clear connection param cache when connection is closed : this was a memory leak before
  • Rather than creating a scheduledExecutor for each telemetry client, we share it across a factory.

Testing

• unit tests

Additional Notes to the Reviewer

• When the LAST connection to a host is closed, all pending telemetry events for that host are flushed across all prior connections (since they all shared the same TelemetryClient). i.e., If you have 5 connections to host-A, closing connections 1-4 does nothing (just decrements refCount). Only when you close connection 5 (the last one) does the flush occur, sending all accumulated telemetry from all 5 connections.

NO_CHANGELOG=true

[tejassp-db]

Connection ref count never decrements to zero. Should be fixed.

[tejassp-db]

Is it possible for the context or connection uuid to be null? If both are always non-null, then we should enforce that in the code.

[samikshya-db]

I am skeptical of removing this because of ThreadLocal usage. Check the internal outage here : [Link]

[tejassp-db]

If a flush is slow, does it block flush for other statements?

[samikshya-db]

Based on my understanding :
I schedule periodic flushes in telemetry client : this is shared across all connections in the process. This submits flush task to executor service (Added a 10 thread case - link).

This should not be blocking other statements.

[samikshya-db]

@tejassp-db - the fixes for Connection ref count never decrements to zero is merged too. thanks for the review.

[tejassp-db]

Is this scenario probable

1. ThreadA - It removes a connection and if branch goes through with empty condition being true.
2. ThreadB - Adds a new connection ConnectionNew to the host
3. ThreadA - Now operates under the incorrect assumption that there are no connections to the host. And will publish incomplete ConnectionNew details and truncate it as well.

[samikshya-db]

[We have already discussed this offline, but adding the pointers for future reference]

The above scenario is not possible. create a connection-> every kind of export will be from TelemetryHelper -> which will in turn get the telemetry client from TelemetryClientFactory.

telemetryClientHolders is a ConcurrentHashMap in the telemetry client factory
ThreadA - It removes a connection and if branch goes through with empty condition being true. -> closes client in sync fashion
ThreadB add a new connection - telemetryClientHolders.compute will not start as threadA has the lock.

@tejassp-db has suggested that we document this behavior in comments. Will add comments to clarify.