[coordinator] Send metric type only if metric type storing is enabled #3033
Conversation
| @@ -496,7 +499,7 @@ func (d *downsamplerAndWriter) writeAggregatedBatch( | |||
| } | |||
|
|
|||
| for _, dp := range value.Datapoints { | |||
| if value.Attributes.PromType != ts.PromMetricTypeUnknown { | |||
| if d.storeMetricType && value.Attributes.PromType != ts.PromMetricTypeUnknown { | |||
There was a problem hiding this comment.
Question: What happens if d.storeMetricType is false and there is no value.Attributes.M3Type - won't that end up in the unknown m3type code path?
Or do we rely on the default value of M3Type being gauge to select gauge? (from the proto M3_GAUGE = 0;)
If we're relying on that behavior perhaps add a comment as such?
There was a problem hiding this comment.
I will check the logic here. My intention here was just to disable the logic which I have added here: https://github.com/m3db/m3/pull/2941/files#diff-bd39ac14b90a7e77e48b860a55e1927d98fd3378a61676da3b11c8b4a7698958R499
| @@ -496,7 +499,7 @@ func (d *downsamplerAndWriter) writeAggregatedBatch( | |||
| } | |||
|
|
|||
| for _, dp := range value.Datapoints { | |||
| if value.Attributes.PromType != ts.PromMetricTypeUnknown { | |||
| if d.storeMetricType && value.Attributes.PromType != ts.PromMetricTypeUnknown { | |||
| switch value.Attributes.PromType { | |||
| case ts.PromMetricTypeCounter: | |||
| err = result.SamplesAppender.AppendCounterTimedSample(dp.Timestamp, int64(dp.Value)) | |||
There was a problem hiding this comment.
Hm I'm a little concerned about sending Prometheus counters to the aggregator with AppendCounterTimedSample since Prometheus counters can actually be in increments that aren't int64 FYI.
i.e. it's totally valid to increment a counter by 0.5 in Prometheus. Notice the API here:
https://godoc.org/github.com/prometheus/client_golang/prometheus#Counter
type Counter interface {
Metric
Collector
// Inc increments the counter by 1. Use Add to increment it by arbitrary
// non-negative values.
Inc()
// Add adds the given value to the counter. It panics if the value is <
// 0.
Add(float64)
}Maybe out of scope of this PR, but we'll have to think about how to pass the metric type to the aggregator without using the AppendCounterTimedSample (and continue to use AppendGaugeTimedSample) I think somehow in the future.
Codecov Report
@@ Coverage Diff @@
## master #3033 +/- ##
=======================================
Coverage 72.2% 72.2%
=======================================
Files 1084 1084
Lines 100091 100091
=======================================
Hits 72297 72297
Misses 22763 22763
Partials 5031 5031
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
Closing this PR in favour of a new PR which takes a different approach: #2941 |
What this PR does / why we need it:
This PR is a minor fix to do not send metric types to aggregator if metric type storing is not enabled.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: