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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
