-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Remove unnecessary functions in PrometheusRemoteWriteExporter #3261
Remove unnecessary functions in PrometheusRemoteWriteExporter #3261
Conversation
dropped++ | ||
errs = append(errs, consumererror.Permanent(err)) | ||
errs = append(errs, consumererror.Permanent(fmt.Errorf("empty data points. %s is dropped", metric.Name()))) |
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.
In a separate PR we should discuss about this, I don't think this should be an error.
if dataPoints.Len() == 0 { | ||
dropped++ | ||
errs = append(errs, consumererror.Permanent(fmt.Errorf("empty data points. %s is dropped", metric.Name()))) | ||
} | ||
for x := 0; x < dataPoints.Len(); x++ { | ||
addSingleIntDataPoint(dataPoints.At(x), resource, metric, prwe.namespace, tsMap, prwe.externalLabels) | ||
} |
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.
I think we can reduce some of the duplicate code here by having a func that accepts IntDataPointSlice. So this and IntSum will call into the same func, as well as for double.
func addIntDataPoints(dataPoints pdata.IntDataPointSlice, ...) error {
if dataPoints.Len() == 0 {
dropped++
return consumererror.Permanent(fmt.Errorf("empty data points. %s is dropped", metric.Name()))
}
for x := 0; x < dataPoints.Len(); x++ {
addSingleIntDataPoint(dataPoints.At(x), resource, metric, prwe.namespace, tsMap, prwe.externalLabels)
}
return nil
}
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.
Actually addSingleIntDataPoint
requires the metric as a parameter. So I both passed datapoints and metric to the new function.
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.
Updated in dcbc7ff
Description:
Modify code as suggested here: #3184 (comment)
Testing:
All unit tests passed.