search

uber-go / tally

A Go metrics interface with fast buffered metrics and third party reporters
MIT License
850 stars 116 forks source link

Terminate timeloop faster for m3 #185

Closed artms closed 2 years ago

artms commented 2 years ago

We quickly start and stop process (it is wrapper for another command) and timeloop potentially can delay process shutdown by up-to 100ms (_timeResolution). Instead of just sleeping we as well wait for signal that shutdown is in progress and quickly terminate loop.

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

artms commented 2 years ago

Production tests fail with errors that timeLoop goroutine is leaking, investigating, I suspect that production is not always properly discarding tally

artms commented 2 years ago

So it seems code is correct, indeed there are a lot of leaking goroutines because tally reporter is not properly closed. So this change potentially can introduce regression for tests which try to detect leaking goroutines.

codecov[bot] commented 2 years ago

Codecov Report

Merging #185 (4d10933) into master (86b6b09) will increase coverage by 0.04%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   47.31%   47.36%   +0.04%     
==========================================
  Files          64       64              
  Lines        5932     5937       +5     
==========================================
+ Hits         2807     2812       +5     
  Misses       2685     2685              
  Partials      440      440
Impacted Files Coverage Δ
m3/reporter.go 92.83% <100.00%> (+0.09%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more