Normalize metrics names #4535
Normalize metrics names #4535
Conversation
EdColeman
commented
May 8, 2024
EdColeman
commented
- updates pool prefix so that pools report as accumulo.pool....
- change names that had names with spaces to use dot notation.
- removed metrics from pools where no user provided properties are used
- updates pool prefix so that pools report as accumulo.pool.... - change names that had names with spaces to use dot notation. - removed metrics from pools where no user provided properties are used
| ExecutorService executor = | ||
| context.threadPools().getPoolBuilder("addSplits").numCoreThreads(16).build(); | ||
| ExecutorService executor = context.threadPools() | ||
| .getPoolBuilder(METRICS_POOL_PREFIX + "table.ops.add.splits").numCoreThreads(16).build(); |
There was a problem hiding this comment.
I don't think this thread pool will emit metrics because metrics are not being turned on in the builder. So this change will just impact the name of the thread as seen in jstack, was that the intention?
There was a problem hiding this comment.
The goal was to try to be consistent across all names. It would make is easier to recognize that it was a pool It seemed adopting a convention would help anywhere the name could show up, logs, jstacks,...
There was a problem hiding this comment.
That sounds like a nice goal. Was asking because the description of the PR was focused on metrics.
| context.threadPools().getPoolBuilder("batch scanner " + batchReaderInstance + "-") | ||
| .numCoreThreads(numQueryThreads).build(); | ||
| queryThreadPool = context.threadPools() | ||
| .getPoolBuilder(METRICS_POOL_PREFIX + "tserver.batch.reader.scanner" + batchReaderInstance) |
There was a problem hiding this comment.
Could drop tserver in the name as these threads run in client code, usually outside if tserver. Could use client instead to indicate a client side thread pool.
| executor = service = context.threadPools().getPoolBuilder("BulkImportThread") | ||
| .numCoreThreads(numThreads).build(); | ||
| executor = service = | ||
| context.threadPools().getPoolBuilder(METRICS_POOL_PREFIX + "bulk.import.service") |
There was a problem hiding this comment.
This is a client side thread pool for bulk load, could use the following name
| context.threadPools().getPoolBuilder(METRICS_POOL_PREFIX + "bulk.import.service") | |
| context.threadPools().getPoolBuilder(METRICS_POOL_PREFIX + "client.bulk.load") |
| ExecutorService threadPool = ThreadPools.getServerThreadPools().getPoolBuilder("estimateSizes") | ||
| .numCoreThreads(numThreads).build(); | ||
| ExecutorService threadPool = ThreadPools.getServerThreadPools() | ||
| .getPoolBuilder("bulk.import.estimate.sizes.pool").numCoreThreads(numThreads).build(); |
There was a problem hiding this comment.
Should this use the METRICS_POOL_PREFIX?
| @@ -45,8 +46,8 @@ public class PropCacheCaffeineImpl implements PropCache { | |||
| public static final int EXPIRE_MIN = 60; | |||
| private static final Logger log = LoggerFactory.getLogger(PropCacheCaffeineImpl.class); | |||
| private static final Executor executor = | |||
| ThreadPools.getServerThreadPools().getPoolBuilder("caffeine-tasks").numCoreThreads(1) | |||
| .numMaxThreads(20).withTimeOut(60L, SECONDS).build(); | |||
| ThreadPools.getServerThreadPools().getPoolBuilder(METRICS_POOL_PREFIX + "caffeine.task") | |||
There was a problem hiding this comment.
Would it be useful to include something in the name that indicates the pool is used for a cache that is related to properties?
| .numCoreThreads(0).numMaxThreads(1).withTimeOut(1, SECONDS) | ||
| .enableThreadPoolMetrics(enableMetrics).build(); | ||
| splitThreadPool = ThreadPools.getServerThreadPools() | ||
| .getPoolBuilder(METRICS_POOL_PREFIX + "tserver.minor.compactor").numCoreThreads(0) |
There was a problem hiding this comment.
I think this is misnamed, should indicate split in the name and not minor compaction.
| @@ -335,31 +340,31 @@ public TabletServerResourceManager(ServerContext context, TabletHostingServer ts | |||
| Property.TSERV_ASSIGNMENT_MAXCONCURRENT, enableMetrics); | |||
| modifyThreadPoolSizesAtRuntime( | |||
| () -> context.getConfiguration().getCount(Property.TSERV_ASSIGNMENT_MAXCONCURRENT), | |||
| "tablet assignment", assignmentPool); | |||
| "tserver.tablet.assignment.pool", assignmentPool); | |||
There was a problem hiding this comment.
Why was this change made? Getting lost in the diffs trying to understand the bigger picture.
