search

tumblr / colossus

I/O and Microservice library for Scala
Apache License 2.0
1.14k stars 96 forks source link

Initialize metrics collection map #540

Closed dxuhuang closed 7 years ago

dxuhuang commented 7 years ago

Assign 0 to the empty TagMap. Wish I could do this in the CollectionMap constructor, but it only accepts params of type T.

https://github.com/tumblr/colossus/issues/449 @DanSimon @benblack86 @jlbelmonte @aliyakamercan @amotamed

codecov-io commented 7 years ago

Codecov Report

Merging #540 into master will increase coverage by 0.04%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
+ Coverage   84.83%   84.87%   +0.04%     
==========================================
  Files          98       98              
  Lines        4312     4317       +5     
  Branches      342      336       -6     
==========================================
+ Hits         3658     3664       +6     
+ Misses        654      653       -1
Impacted Files Coverage Δ
.../main/scala/colossus/metrics/collectors/Rate.scala 83.87% <100%> (+2.38%) :arrow_up:
...s/src/main/scala/colossus/metrics/Collection.scala 100% <100%> (ø) :arrow_up:
...in/scala/colossus/metrics/collectors/Counter.scala 81.81% <100%> (+0.86%) :arrow_up:
.../scala/colossus/metrics/collectors/Histogram.scala 92.64% <100%> (+0.73%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e6721e8...575ad93. Read the comment docs.

DanSimon commented 7 years ago

@benblack86 I mostly agree with your comments. I'm fairly certain the issue was created after a discussion about the errors metric and the fact that it can be confusing when sometimes the metric shows up as 0 vs not at all.

The only thing to keep in mind is service/errors is a built-in Colossus metric. So in that case "initialize to zero on startup" would have to be something we hardcode into colossus itself. Maybe that's what we want, but we have to be really careful that it's a viable solution for all use cases.

Also since it seems like we're not fully settled on what to do, maybe we should move discussion into the issue itself and make a final decision before we make any more changes here.

aliyakamercan commented 7 years ago

I agree with @benblack86 on #449. I don't think this is an issue and we don't have a solution which covers all use cases.