srdo commented on PR #17139:
URL: https://github.com/apache/kafka/pull/17139#issuecomment-2590986916

   You are completely correct. My mistake. TimeRatio can't "lose time" the way 
I thought. The scenario I described was wrong, I misunderstood the code. 
   
   So if the two other concerns mentioned above (thread safety, the effect of 
multiple metrics reporters) turn out to not be relevant, I guess TimeRatio is 
fine as is?
   
   Not sure if this matters in practice, but here's another example 
demonstrating funny metric outputs:
   
   ```
       @Test
       public void testPartialPoll() {
           MetricConfig config = new MetricConfig();
           MockTime time = new MockTime();
           TimeRatio ratio = new TimeRatio(1.0);
   
           ratio.record(config, 0.0, time.milliseconds());
           time.sleep(10);
           // Pretend poll starts here
           time.sleep(10);
   
           //This emits the default value because the poll isn't done, so the 
current window has size 0
           assertEquals(1.0, ratio.measure(config, time.milliseconds()));
   
           time.sleep(10);
           ratio.record(config, 20, time.milliseconds());
   
           // This is technically the correct value, but this measurement ends 
up covering the entire time interval
           // and we're still left with a meaningless 1.0 metric value above.
           // When a user looks at this in their metrics, they'll see 1.0 
followed by 0.66, which is pretty hard to interpret.
           assertEquals(2 / 3.0, ratio.measure(config, time.milliseconds()));
           
           // To demonstrate that this can be repeated
           // Pretend another poll starts here
           time.sleep(10);
           // We again get a meaningless 1.0 metric since the window is 0 in 
size
           assertEquals(1.0, ratio.measure(config, time.milliseconds()));
           time.sleep(10);
           ratio.record(config, 10, time.milliseconds());
           // followed by the "real" value in this call
           assertEquals(0.5, ratio.measure(config, time.milliseconds()));
       }
   ```
   
   In this test, the user will observe the following emitted metric values: 
1.0, 0.66, 1.0, 0.5, even though two of those values are meaningless. This 
happens if `measure` is called twice in a row without a completed poll between 
them. 


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

Reply via email to