[GitHub] [pulsar] asafm commented on pull request #15558: [fix][broker][functions-worker] Ensure prometheus metrics are grouped by type (#8407, #13865)

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

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

   @tjiuming Maybe you meant @marksilcox ?
   
   I have a note regarding the way we write `NamespaceStatsAggregator`. 
   The whole idea in the way the metric system is built today in Pulsar (I know it's fractured) is to try to avoid memory allocation as much as possible. It does so by:
   * Using `long` and `AtomicLongFieldUpdater` instead of `AtomicLong`
   * Writing the values directly into ByteBuf and then to the stream. 
   * In some cases directly into the stream
   
   In your refactor, we add a Sample object each containing 2 lists and a value (Double). On the way, we create a Map, and of course a `MetricFamilySamples` object. The sample is done per metrics we have. 
   I'm just starting to learn the metrics code in Pulsar, but I think it beats the point of the original code. I believe @merlimat can correct me.
   
   What I suggest is to build the code to achieve the goal you desire, each sample belonging to a specific metric will be written one after another. 
   
   How?
   
   Let's collect all `TopicStats`. We pay the cost of allocating them anyway. Once we have that, we can write a function writing each individual metric samples. Each will call a function that looks like:
   `writeMetric(stream, "msgsRate", cluster, namespace, topicToStatsMap, topicStats -> topicStats.msgsRate)`
   
   
   WDYT?


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