Do not require MetricReader to have ForceFlush

[pellared]

Supersedes (prior art) #3563

Why: #3563 (comment)

By @reyang

"what's the difference between MetricReader.Collect and MetricReader.ForceFlush, and why do we need MetricReader.ForceFlush"? My gut feeling is that we don't.

After reading the a few times and reviewing Go Metrics SDK I think it is correct that it is not needed in the specification.

In Go Metrics SDK ForceFlush implementation is basically only delegating the calls to the push exporter or doing nothing.

ForceFlush is needed only for MetricProvider and Push Metric Exporters.

Take notice that MetricReader operations does not define ForceFlush. OTel .NET SDK reader's DO NOT implement ForceFlush. As far as I understand the MetricReader can still have ForceFlush, but it is a detail which is left to the SDK implementation how metrics SDK accomplishes the goal of MetricProvider.ForceFlush.

Related (currently blocked) PR in Go SDK: open-telemetry/opentelemetry-go#4375

[Oberon00]

It seems this part is "Stable" so we cannot remove functionality. I think it would be OK to recommend any new implementation / major version to do so though.

Also, if the two functions are redundant, my gut feeling would be to keep ForceFlush, since that name is consistent with other signals.

[pellared]

@Oberon00

It seems this part is "Stable" so we cannot remove functionality. I think it would be OK to recommend any new implementation / major version to do so though.

What is theMetric.Reader functionality? What is it doing? I do not see it defined (as @reyang noted). I see this PR as a "fix" for a Stable documentation. This is not removing any method from an interface as the specification does not define the exact API for the readers.

EDIT: Maybe the entry in changelog should be written differently. See 63c70dc

Also, if the two functions are redundant, my gut feeling would be to keep ForceFlush, since that name is consistent with other signals.

I do not follow.
I am not changing any name.

[Oberon00]

OK, then probably I misunderstood something, I thought Collect was doing what ForceFlush would do. I'm not very familiar with metrics, please excuse the confusion.

[brettmc]

"what's the difference between MetricReader.Collect and MetricReader.ForceFlush, and why do we need MetricReader.ForceFlush"? My gut feeling is that we don't.

Wouldn't MetricReader.ForceFlush call MetricReader.Collect and then call ForceFlush on any exporters associated with the reader?

[pellared]

"what's the difference between MetricReader.Collect and MetricReader.ForceFlush, and why do we need MetricReader.ForceFlush"? My gut feeling is that we don't.

Wouldn't MetricReader.ForceFlush call MetricReader.Collect and then call ForceFlush on any exporters associated with the reader?

@brettmc That was the original scope of #3563 but it was rejected. Take notice that e.g. OTel .NET SDK reader's DO NOT implement ForceFlush. As far as I understand the MetricReader can still have ForceFlush, but it is a detail which is left to the SDK implementation.

[jsuereth]

This would be a breaking change to the specification.

Basically you're changing the registration requirements of the SDK form tracking MetricReaders registered to tracking MetricExporters. However, this also doesn't expose any mechanism for MetricExporters to be registered on the SDK.

See the "Configuration" section of the MeterProvider.

[pellared]

@jsuereth

Since MetricReader is exposed as something users could implement

AFAIK it was never told nor specified.

CC @open-telemetry/go-maintainers as the interface in Go SDK is "internal" (contains unexported methods that users cannot implement without hacking).

This is the design in MOST languages, and was the intention.

I cannot confirm if this was the intention. The metrics SDK specification is extremely open to interpretation 🤷

Do you think that #3563 is the desired change?

[reyang]

I think the current specification is okay but not great.

Ultimately, we will have to revisit #1432, and IIRC Collect and ForceFlush were introduced with the future consideration that collection cycle can be different than the export cycle.

@pellared would you be interested / have bandwidth to address #1432? If not, my gut feeling is that most likely we will just leave the spec "as-is" for now.

[jsuereth]

AFAIK it was never told nor specified.

I agree the specification is obtuse here. See this section on Pull-based exporters:

Implementors MAY choose the best idiomatic design for their language. 
For example, they could generalize the [Push Metric Exporter interface](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#push-metric-exporter) design 
and use that for consistency, they could model the pull exporter as 
[MetricReader](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricreader),
or they could design a completely different pull exporter interface. 
If the pull exporter is modeled as MetricReader, implementors 
MAY name the MetricExporter interface as PushMetricExporter to prevent naming confusion.

[jsuereth]

Do you think that #3563 is the desired change?

I did approve this change. I'm also ok taking some time to untangle the specification here. There are two pieces that are important to me:

1. All MetricReader instances are registered with a MeterProvider. This allows the actual mechanism of storing/exporting metrics to be optimised based on configuration.
2. Users have a mechanism to ForceFlush metrics through whatever pipelines we've defined to MetricExporters. Yes this only makes sense for push-based, but the registration should be done using a MeterProvider and affect all configured push exporters. This alleviates issues, e.g. w/ batch compute or FAAS where you want to flush-on-death or flush-after-request.

The way this current PR is implemented removes explicit wording for (1).

[pellared]

The way this current PR is implemented removes explicit wording for (1).

@jsuereth, makes sense. WDYT of 6d3a25a + c538b91?

Ultimately, we will have to revisit #1432, and IIRC Collect and ForceFlush were introduced with the future consideration that collection cycle can be different than the export cycle.

@pellared would you be interested / have bandwidth to address #1432? If not, my gut feeling is that most likely we will just leave the spec "as-is" for now.

@reyang, I think that #1432 has a bigger scope. I would prefer to make a baby steps to improve the current state of the specification. I think it is better when I propose smaller changes as otherwise my contributions would be extremely ineffective because I was not participating in creating of the metrics SDK. At last, I am often unable to join the Specification SIG meeting. Still I will take a look at the issue 😉

@jsuereth, @reyang, thanks for your patience and reviews 👍

[jsuereth]

@jsuereth, makes sense. WDYT of 6d3a25a + c538b91?

I like the second, the first I think is as confusing as the existing specification on this.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: pellared / name: Robert Pająk (0503d08, ba8ec0d, 0c687e3, 9406b13, 6f2b5c7, 5d5707e, d32b2b9, 580a3d2, 63c70dc, 8c8a2db, 5423791, 6d3a25a, 7e78290, bcdada0, e9f6c68, c538b91, 8850ba9, 6887614, 58baadc)

[jsuereth]

I'm removing my request for changes on this PR, However, I'm not supportive of the change overall.

1. While this removes ForceFlush from MetricReader, it actually creates a tight coupling between the ownership model of MetricReader, MetricExporter and MeterProvider. These three things need to fully understand each other, where the previous wording of the spec basically only needed MeterProvider<->MetricReader and MetricReader<->MetricExporter.
2. I think the link from MeterProvider->MetricExporter is now implicitly defined by the specification and therefore likely to cause as much confusion as other implicitly-assumed connections in the specification. Yes this could enable some flexibility in implementation, but possibly at the cost of understandability in a specification that already struggles to be understandable / simple.

Enough changes have been made that this proposal is now consistent for the specification, but I'm still against the change overall.

[pellared]

During the SIG meeting we agreed that this PR does not make the specification more "normative". While it was acceptable the "fuzziness" of the specification would remain. We decided to revisit #3563 and make sure it does not conflict with #1432. We also agreed that we should not to solve too much in a single PR (e.g. solve #1432) but just make sure to improve the ForceFlush descriptions. We also agreed that the fact that .NET readers do not implement ForceFlush should not a blocker for #3563.