feat(host-metrics): add 'metricGroups' configuration option to limit which metrics are collected

[trentm]

Collection of some host-metrics metrics can be costly. Using a Metrics View can be used
to drop metrics, but there will still be the cost of having collected them in the
first place. This adds a config option to select which groups of metrics should
be collected.

Checklist

• tests

[legendecas]

Thanks, looks good to me!

[trentm]

@legendecas Are you able to review this again, now that I've added some tests? Thanks.

[pichlermarc]

Non-blocking: I wonder if the intention of the spec was to actually NOT to invoke async observable callbacks in the first place if a Drop aggregation is specified for a given instrument (via View or MetricReader).

Considering that the main use-case for Observable instruments is to allow expensive operations to only run when the MetricReader triggers a collect operation, it would make sense to just not invoke the callback to begin with if a Drop aggregation is configured. AFAIK we don't do it like that in the metrics SDK right now, but technically not observing it at all wouldn't make a difference and would solve this for every observable instrument that suffers from the same issues.

[pichlermarc]

anyway - non-blocking: making such a change is likely a huge undertaking. I just checked our implementation and it would be really difficult to make that change happen the way things are implemented today.

[legendecas]

@pichlermarc there might be multiple views configured on a single metric instrument like an observable.

I think it could be considered as an optimization rather than a requirement that if there is only one metric view of drop aggregation configured for an observable, the callback of the observerble could be skipped.

However, on https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#observations-inside-asynchronous-callbacks:

Callback functions MUST be invoked for the specific MetricReader performing collection.

[pichlermarc]

@pichlermarc there might be multiple views configured on a single metric instrument like an observable.

I think it could be considered as an optimization rather than a requirement that if there is only one metric view of drop aggregation configured for an observable, the callback of the observerble could be skipped.

Yep, agree.

However, on https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#observations-inside-asynchronous-callbacks:

Callback functions MUST be invoked for the specific MetricReader performing collection.

Mhm, looking at the whole paragraph:

Callback functions MUST be invoked for the specific MetricReader
performing collection, such that observations made or produced by
executing callbacks only apply to the intended MetricReader during
collection.

It seems like it mostly concerns itself with observations not cross-polluting other MetricReaders collection cycle. I'm not sure I could make a good argument why applying the optimization would be spec-compliant.

Digging for precedent in Java, there's this:

• no storage is ever registered if a Drop aggregation is configured: https://github.com/open-telemetry/opentelemetry-java/blob/39e60f818a430085cf0891e03ce4f4927f84df6d/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeter.java#L314-L316
• the callback is never invoked when there are no storages: https://github.com/open-telemetry/opentelemetry-java/blob/39e60f818a430085cf0891e03ce4f4927f84df6d/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/CallbackRegistration.java#L74-L77

Unfortunately there was no discussion on that specific behavior in the PR that added it.

Since our implementation is similar, I now wonder if there's a chance that we could do something similar

• not registering a storage for Drop aggregations here: https://github.com/open-telemetry/opentelemetry-js/blob/a8b4cd2b9d7b41e08ee4eaedb15fc61ea26201ad/packages/sdk-metrics/src/state/MeterSharedState.ts#L143
• and then returning early in some of the code invoked here https://github.com/open-telemetry/opentelemetry-js/blob/a8b4cd2b9d7b41e08ee4eaedb15fc61ea26201ad/packages/sdk-metrics/src/state/MeterSharedState.ts#L80-L118
  • probably somewhere in ObservableRegistry

Views would then be able to prevent callbacks from being invoked - we'd have to change the BatchObservable to a per-instrument Observable here though.

[legendecas]

I think a dedicated issue is worthwhile for this in the core repo. Would you like to open an issue in the core repo?

[pichlermarc]

I think a dedicated issue is worthwhile for this in the core repo. Would you like to open an issue in the core repo?

done open-telemetry/opentelemetry-js#6039