search

samstarling / finagle-prometheus

Provides a bridge between Finagle and Prometheus metrics
https://samstarling.co.uk/projects/finagle-prometheus/
MIT License
30 stars 18 forks source link

bump finagle version and replace HashedWheelTimer with DefaultTimer #40

Closed hderms closed 5 years ago

hderms commented 5 years ago

fix #39

Not sure this is the approach we want. I got a NPE when running the tests which seemed to be around the com.twitter.app.LoadService machinery. That being said https://github.com/twitter/finagle/blob/develop/finagle-stats-core/src/main/scala/com/twitter/finagle/stats/MetricsStatsReceiver.scala#L180 uses the DefaultTimer so I'm at a loss as to what we would do otherwise.

samstarling commented 5 years ago

Hi @hderms: thanks for the contribution, and sorry to hear you'd been having trouble. I also had a lot of trouble in the past with the LoadService mechanism, and flapping unit tests. Looks like they passed though, so I'll merge this. I'll try my best to get a release out tonight, or if not then tomorrow.

hderms commented 5 years ago

@samstarling if you think it's worth the risk then I'm cool. I'm trying to do a double check on the finagle gitter to make sure DefaultTimer is what we are indeed supposed to be using.

samstarling commented 5 years ago

Sounds good – let me know if you need anything from me: it looks like the tests also passed on master after I merged, but I'll run them a few more times. If we fixed those sporadic failures, I see it as a victory, and I'd be interested to see what you get when you try the new version of the library when it's released.