-
Notifications
You must be signed in to change notification settings - Fork 612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Filter out metricCount=0 and its corresponding metricValue for service connect metric TargetResponseTime #3537
Filter out metricCount=0 and its corresponding metricValue for service connect metric TargetResponseTime #3537
Conversation
ea109f7
to
74cc444
Compare
5660edf
to
7ca989b
Compare
7ca989b
to
c884cb1
Compare
943121b
943121b
to
2cf0d58
Compare
…e connect metric TargetResponseTime
c00164b
2cf0d58
to
c00164b
Compare
metricValues, metricCounts = convertHistogramMetricCounts(metricValues, metricCounts) | ||
|
||
// If all values are 0 in metricCount, then no need to send the metrics to TACS | ||
if len(metricCounts) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an optimization we can pre-compute this by checking the SampleCount value to be 0 (metric.Histogram.SampleCount
I think) on L99 and completely skip processing these completely empty buckets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. I would take this up as a follow-up task and address that in a separate PR.
This PR filters out all metric Counts for the histogram metric TargetResponseTime which are 0. We also remove the corresponding metricValues.
This is because when there is no traffic and metricCount is 0, the dashboards incorrectly show min as 0 and max as 360000. In this case, there should have been no datapoints. For example,
Before:
MetricCounts: [8, 0, 3, 0, 0, 0, 1, 0, 0, 0, 0, 0,0, 0, 0, 0, 0, 0, 0 ]
MetricValues: [ 0.5, 1, 5, 10, 25, 50, 100, 250, 500, 1000, 2500, 5000, 10000, 30000, 60000, 300000, 600000, 1.8e+06, 3.6e+06 ]}
This Pr correctly sends only non-zero metric counts and now it looks like below.
After:
MetricCounts: [8, 3, 1]
MetricValues: [0.5, 5, 100]}
Summary
Implementation details
Testing
New tests cover the changes: yes
In the summary the test results are mentioned
Description for the changelog
BugFix: Filter out metricCount=0 and its corresponding metricValue for service connect metric TargetResponseTime
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.