-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
hostmetricsreceiver: refactor load metrics to use generated metrics #2375
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #2375 +/- ##
==========================================
- Coverage 92.01% 92.00% -0.01%
==========================================
Files 273 272 -1
Lines 15434 15405 -29
==========================================
- Hits 14202 14174 -28
Misses 850 850
+ Partials 382 381 -1
Continue to review full report at Codecov.
|
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.
lgtm
@@ -24,7 +24,7 @@ import ( | |||
// Type is the component type name. | |||
const Type configmodels.Type = "hostmetricsreceiver" | |||
|
|||
type metricIntf interface { | |||
type Metric interface { |
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.
- Does this need to be exported? (It was not previously).
- Is it possible to come up with a less generic name? Looking at the name
Metric
I have a hard time understanding what it is supposed to do.
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.
@tigrannajaryan I had changed it to exported to be able to pass through the interface so the Init() function could be called generically -
opentelemetry-collector/receiver/hostmetricsreceiver/internal/scraper/loadscraper/load_scraper.go
Lines 69 to 76 in db35667
initializeLoadMetric(metrics.At(0), metadata.Metrics.SystemCPULoadAverage1m, now, avgLoadValues.Load1) | |
initializeLoadMetric(metrics.At(1), metadata.Metrics.SystemCPULoadAverage5m, now, avgLoadValues.Load5) | |
initializeLoadMetric(metrics.At(2), metadata.Metrics.SystemCPULoadAverage15m, now, avgLoadValues.Load15) | |
return metrics, nil | |
} | |
func initializeLoadMetric(metric pdata.Metric, metricDescriptor metadata.Metric, now pdata.TimestampUnixNano, value float64) { | |
metricDescriptor.Init(metric) |
I think pattern will occur more as more of the metrics in the receiver are refactored to be based off the generated metrics. The previous use cases were only for the scrapers that emitted a single metric.
I'm indifferent towards the name - MetricIntf
? MetricFactory
? GeneratedMetric
?
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.
OK, understood re exporting.
As for the name, please use something that reflects the purpose. I do not quite understand what it does to suggest a good name. The generic name Metric
does not help me to understand what it is doing.
If unsure keep it unchanged.
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 reverted the name back to MetricIntf
for now and added a comment above to describe it.
Description: Adding the load metrics to the metadata.yaml, and refactoring the hostmetricreceivers load metrics to use generated metrics