ThreadStats - Send correct metric type instead of always sending Gauge #242
Conversation
…e right metric type
nilabhsagar
changed the title
|
The above pull request will fix the issue #245 |
|
@nilabhsagar looks like this failed the Travis build for Python 2.5, but only for line-length. Are you still looking at this? |
|
@jayshao Yes, I can fix it. Could you help me with the exact line number to fix. |
|
@jayshao Its fixed now, All Travis build passed. |
|
Hi, Requesting someone to review and merge with the master? |
|
@yannmh Request you to review. |
|
Please merge ASAP. Thanks @nilabhsagar for fixing this enormity ;-) |
|
@nilabhsagar Thanks for reaching out. We have been looking into your PR, and unfortunately it is not mergeable as is. Currently the server side API would also require changes to support some of these metric types. I expect we'll be able to provide more insight into viability and/or timeline next week. |
|
@irabinovitch Sure, Let me know when some steps are required from my side. |
|
Thanks for working on a fix @nilabhsagar 🙇 I apologize for the late reply. Your changes look good, and so do the tests.
Please let me know if you'd like make these changes, or have us add them to your existing PR. |
…tric_type # Conflicts: Resolved # datadog/threadstats/base.py
… to update the metric metadata accordingly and makes it able to perform time rollup operations. 2. Changed metric type to rate instead of count.
|
@yannmh I have done the changes based on your suggestion. I hope it is aligned with your suggestion. Do let me know if I am missing something. Please verify the commit "7cb37c9" as others are just line wrapping issue fix. |
datadog/threadstats/metrics.py
Outdated
| count = sum(self.count, 0) | ||
| return [(timestamp, count, self.name, self.tags, self.host)] | ||
| return [(timestamp, count, self.name, self.tags, self.host, MetricType.Rate, interval)] |
There was a problem hiding this comment.
We want to replace count by count/float(interval) to report a proper rate.
datadog/threadstats/metrics.py
Outdated
| self.tags, self.host, MetricType.Gauge, interval), | ||
| (timestamp, self.max, '%s.max' % self.name, | ||
| self.tags, self.host, MetricType.Gauge, interval), | ||
| (timestamp, self.count, '%s.count' % self.name, |
There was a problem hiding this comment.
Same as above. We need to transform the count to a proper rate.
|
@nilabhsagar added a small comment. Other than that, it looks good! |
|
@yannmh Please review. |
|
Thank you @nilabhsagar. This will be part of our next release, which will go out this week. |
ThreadStats - Send correct Metric Type instead of always sending Gauge
Problem
At present in ThreadStats various types of metric gets created like gauge, count, histogram, etc. but at the time of flushing for each metric type is set to "gauge". This is a problem as sending count from server becomes gauge in the web. Changing the type in web will not have any effect as from the server it is always coming as gauge.
Solution
Assign the right metric type based on the metric functionality.