[sdk-metrics] consider not invoking observable callbacks when instrument is configured with a `Drop` aggregation

[pichlermarc]

Description

Important

This issue is intended to investigate feasibility of an optimization for Observable Instruments. It is a prerequisite before implementing it.

The specification mentions the following:

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. Since the main use-case for Observable Instruments is to delay expensive metrics generation until the collection is triggered, it seems reasonable to assume that we could not invoke the callback if the instrument is configured to use only Drop aggregations.

Views would then be able to prevent callbacks from being invoked by setting a Drop aggregation.

Digging for precedent in Java, there's this:

• no storage is ever registered if a Drop aggregation is configured
• the callback is never invoked when there are no storages:

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:

  opentelemetry-js/packages/sdk-metrics/src/state/MeterSharedState.ts

  Line 143 in a8b4cd2

  const aggregator = view.aggregation.createAggregator(viewDescriptor);

• and then returning early in some of the code invoked here

  opentelemetry-js/packages/sdk-metrics/src/state/MeterSharedState.ts

  Lines 80 to 118 in a8b4cd2

  	async collect(
  	collector: MetricCollectorHandle,
  	collectionTime: HrTime,
  	options?: MetricCollectOptions
  	): Promise<ScopeMetricsResult | null> {
  	/**
  	* 1. Call all observable callbacks first.
  	* 2. Collect metric result for the collector.
  	*/
  	const errors = await this.observableRegistry.observe(
  	collectionTime,
  	options?.timeoutMillis
  	);
  	const storages = this.metricStorageRegistry.getStorages(collector);
  	// prevent more allocations if there are no storages.
  	if (storages.length === 0) {
  	return null;
  	}
  	const metricDataList = storages
  	.map(metricStorage => {
  	return metricStorage.collect(collector, collectionTime);
  	})
  	.filter(isNotNullish);
  	// skip this scope if no data was collected (storage created, but no data observed)
  	if (metricDataList.length === 0) {
  	return { errors };
  	}
  	return {
  	scopeMetrics: {
  	scope: this._instrumentationScope,
  	metrics: metricDataList,
  	},
  	errors,
  	};
  	}

  • probably somewhere in ObservableRegistry

This issue is considered done when:

• a specification issue has been opened, asking for clarification if this approach is allowed and a response was received
• (if the response was positive) feasibility of implementing such a change was shown via a Draft PR
  • (can be done in parallell, but the draft will not be considered for merging unless spec clarification - either through issue comment or PR - was completed first)