[GitHub] [pulsar] asafm commented on pull request #16758: [improve][txn] PIP-160 Metrics stats of Transaction buffered writer

[GitBox <gi...@apache.org>]

asafm commented on PR #16758:
URL: https://github.com/apache/pulsar/pull/16758#issuecomment-1207782630

   > Hi @asafm
   > 
   > > As I understand, this only happens once per instance of MLTransactionLogImpl. So I don't understand why you want to pass metric by metric. Something doesn't add up here.
   > > I guess my main question is: how many MLTransactionLogImpl are there in a single broker process? More than 1?
   > 
   > Yes, maybe more than one.
   
   Ok, if more than one, then the design must change. I thought the whole idea was that you have a single instance of metrics per metric prefix.
   If not, after much thought I suggest the following:
   ```java
   abstract class BufferMetrics {
   	protected abstract void observeRecordsPerBatch(int)
   	protected abstract void incFlushTriggeredByMaxRecords(int)
   }
   
   MLTransactionMetadataStoreBufferedWriterMetrics extends BufferMetrics {
   	static private Histogram recordsPerBatchMetric = new Histogram.Builder()
                           .name("pulsar_tx_store_bufferedwriter_batch_record_count")
                           .labelNames(new String[]{"txCoordinatorId"})
                           .help("Records per batch histogram")
                           .buckets(RECORD_COUNT_PER_ENTRY_BUCKETS)
                           .register(registry));
   	
       private Histogram.Child recordsPerBatchHistogram;                      
   
   
   	public MLTransactionMetadataStoreBufferedWriterMetrics(String txCoordinatorId) {
   	    recordsPerBatchHistogram = recordsPerBatchHistogram.labels(txCoordinatorId)
   
   	}
   }
   ```
   
   The pros:
   * It's explicit
   * No confusing pass of label names multiple times which after 2nd time are not really used.
   
   The cons:
   * A bit awkward
   
   Another approach which I disliked a bit, but it's still ok:
   Add to Pulsar Common:
   ```java
   class PrometheusRegistryChecker {
        static defaultMetricRegistryNameToCollector = new HashMap<String, Collector>()
   
        static Collector registerIfNotExists(collector) {}
   }
   ```
   Like `FunctionCollectorRegistryImpl`
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org