Attribute sets not observed during async callbacks are not exported

[dashpole]

Part of #2711

Non-Goals

This is limited to asynchronous instruments in order to minimize the changes required for SDKs. It could be extended to synchronous instruments in the future with a Delete() function, as requested in #3062. Implementing this specification would allow temporarily working around the limitation for synchronous instruments in some cases as described in #3062 (comment), and would validate the approach recommended here before making the addition to the API.

This does not impose additional requirements on the SDK's handling of start time.

Changes

This updates the metric SDK specification to recommend against producing aggregated metric data for attribute sets which are not observed during a callback. This helps the use-cases below by allowing the SDK to free memory for non-observed attribute sets, and allows for the intentional removal of series by omitting them from the callback.

It also adds to the supplementary guidelines to describe how to handle start time for attribute sets which disappear and reappear if the SDK tracks per-timeseries start time.

Use-cases

Memory management: As described in open-telemetry/opentelemetry-go#3605 (comment), and #1891, infinite cardinality recordings can cause memory problems if all previously observed attribute sets are kept in-memory, and continue to be exported. If SDKs are not expected to produce aggregated data for non-observed attribute sets, they are able to limit memory to only what is required by actively-reported attribute sets when using async instruments.

Intentional Removal of Series: As described in #3062 (comment), a user may want a particular series to cease being reported to their backend.

Related Issues:

• Metrics SDK: define standard circuit breakers for uncollected, infinite cardinality recordings #1891
• Support explicitly removing attributes from an instrument API #3062
• Support recording metrics with "churning" labels without "leaking" memory. opentelemetry-go#3605

cc @asafm @MrAlias @jkwatson @jack-berg @reyang @jmacd

[dashpole]

I'm out on leave for a while. Others can feel free to contribute the changes, or i'll pick it back up when I'm back.

[dashpole]

I'm back from leave, and would like to pick this back up. Reviews are appreciated

[asafm]

@dashpole The change described in this PR is what the Java SDK is doing today. So this PR makes this explicit? I thought that all SDKs "forgot" the previously recorded points.

[dashpole]

So this PR makes this explicit?

Yes

I thought that all SDKs "forgot" the previously recorded points.

Golang does not (see open-telemetry/opentelemetry-go#3605, and the original comment thread, open-telemetry/opentelemetry-go#3549 (review)). Part of the rationale was that the spec was silent on whether non-observed attribute sets should be exported, which this PR attempts to clarify. I can look through other SDKs to see what other SDKs do.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[MrAlias]

@carlosalberto should this also be included in the upcoming release?

I think this is a non-controversial change that is just explicitly stating what is already being done. It should be ready to merge right?

[trask]

just checking, is this PR still needed now with the new clarification being added in #3540?

For asynchronous instruments with Delta or Cumulative aggregation temporality, MetricReader.Collect MUST only receive data points with measurements recorded since the previous collection.

[dashpole]

The updates to the supplementary guidelines are still helpful. The language in sdk.md isn't needed after that change.

[reyang]

@dashpole would you move the changelog entry to unreleased?

[dashpole]

@reyang updated the changelog