chore(host-metrics): solve some TODO comments #124
Conversation
| @@ -206,27 +214,41 @@ class SystemCpuUtilizationAggregation extends Aggregation { | |||
| /** @type {HostMetrics} */ | |||
| let hostMetricsInstance; | |||
| function enableHostMetrics() { | |||
| // TODO: make this configurable, user might collect host metrics with a separate utility | |||
| // NOTE: we set the scope name to the package name `@opentelemetry/host-metrics` like | |||
There was a problem hiding this comment.
Note for reviewer: maybe this is a hot take but IMO if we let the user to name the scope of host metrics as he/she wants we wont have a clear way to know if the metrics came from @opentelemetry/host-metrics or another utility/package
There was a problem hiding this comment.
Do you see any value in passing in a value here, or having it configurable? I don't, but I may be missing soemthing. If not, then this whole function could be:
function enableHostMetrics() {
hostMetricsInstance = new HostMetrics();
hostMetricsInstance.start();
}There was a problem hiding this comment.
removing it since it has a default value
| new View({ | ||
| instrumentName: 'system.cpu.time', | ||
| aggregation: Aggregation.Drop(), | ||
| }), | ||
| // use the aggregation we craeted above | ||
| // drop `process.*` (not in Kibana) | ||
| new View({ |
There was a problem hiding this comment.
Note for reviewer: dropping process.* metrics for now since there is no UI showing them. We probably want an aggregation for process.cpu.utilization but I think it would be easier and shorter once open-telemetry/opentelemetry-js#4616 is done
…-otel-node into dluna/host-metrics-todos
This PR address a couple of
TODOcomments for metrics. I proposed a couple of changes that are meant to be discussed here before the alpha comes out.