void-ptr974 commented on PR #25766: URL: https://github.com/apache/pulsar/pull/25766#issuecomment-4519952629
I think we should separate the bug fix from the metrics exposure work. For this PR, the minimal and correct fix is to change async multi() from createStats to multiStats and add a focused test verifying that async multi records into multiStats rather than createStats. The later changes introduce a new metadata-store metrics exposure path by passing a BookKeeper StatsProvider through MetadataStoreConfig. That seems like a separate design discussion, and it also expands the dependency on the BookKeeper stats provider in the metadata-store layer, which has already been pointed out in review. If we want to expose metadata-store/ZK operation metrics to Prometheus, I think we should do it in a follow-up PR with a Pulsar-owned metrics abstraction, or have broker-level Prometheus code export metadata-store operation metrics directly. The metric should probably be modeled as operation-level metrics, e.g. operation=multi, provider=zk, store=local/configuration, instead of encoding everything into the metric name. Also, the test should not only assert that zk_multi metric names exist. The metric may exist because the OpStatsLogger was created during initialization. It should assert that async multi actually increments multiStats and does not increment createStats. -- 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]
