[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-1200442194

   I started writing a few comments then I realized I have some profound suggestions I thought maybe it's a better idea to raise them first. 
   
   I think currently this PR in my opinion quite complicated. It took me 2 hours to get the hang of it all.
   Here is what I suggest IMO to make it a bit simpler:
   
   1. There is a lot of logic related to removing labels once the buffered writer is closing. It's even a bit more complicated as some buffered writers share the same label values, so they have one child of the histogram collector for example, but many usages. It's very complicated. 
   
   BufferedWriter is used on two occasions: Pending Ack stores, and Transaction Log. 
   Transaction Log doesn't need that complicated logic as it creates a single Buffered Writer.
   Pending Ack Stores does. Since it has a provider class, we can make this logic "applicative" and place it in the `MLPendingAckStoreProvider`. 
   
   How?
   BufferedWriter will get a BufferedWriterMetrics in the constructor.
   BufferedWriterMetrics will get BufferedWriterMetricsConfiguration in its constructor.
   
   BufferedWriterMetricsConfiguration is the same as the definition you have:
   `labelNames`, `labelValues`, `metricsPrefix` (you called it `component`) and `enabledMetrics`.
   
   
   Since you said the pending ack stores will share the same labels, but won't be differentiated by the label values, you can create a single BufferedWriterMetrics instance and use it whenever you create a new Buffered writer.
   
   When a ledger is closed, its buffered writer is closed.
   BufferedWriterMetrics will be closed by its creator: `MLPendingAckStoreProvider` will know when to close it's the only instance since it can easily keep track or already have the number of open ack stores. `TransactionLog` will keep its only instance and will close it upon closing the managed ledger / buffered writer.
   
   2. I wouldn't bother with unregistering the metric. It's only relevant when there are no pending ack stores. The only cost in this case: is 2 lines emitted in the Prometheus output (help and type) since there are no samples to print.
   
   3. In BufferedWriter Metrics init, I would use the same optimization trick I saw at `FunctionStatsManager`: 
    step 1: create all metrics: histogram, collectors, etc. mainly supplying labels names.
   step 2: create the child of each collector, by supplying the label values. Save it as a variable and use it.
   
   4. In close(), just remove the labels from the collector.
   5. One larger change I would do: have BufferedWriterMetrics class have an action method: batchFlushedTriggedByForceFlush(), and its arguments containing anything you need for your metrics update. hide everything inside, including the `appendistogram` you have there. 
   6. I would get rid of the disabled static instance, and simply do nothing upon each action method in BufferedWriterMetrics if `metricsEnabled` is false. Encapsulate it.
   
   I have many more comments, but I thought it's best to discuss the big ones first.
   
   
   
   
   
   


-- 
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