-
Notifications
You must be signed in to change notification settings - Fork 888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not require MetricReader to have ForceFlush #3609
Conversation
MetricProvider.ForceFlush
MetricProvider.ForceFlush
MetricReader.ForceFlush
MetricReader.ForceFlush
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. |
What is the EDIT: Maybe the entry in changelog should be written differently. See 63c70dc
I do not follow. |
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. |
Wouldn't |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a breaking change to the specification.
Basically you're changing the registration requirements of the SDK form tracking MetricReader
s registered to tracking MetricExporter
s. However, this also doesn't expose any mechanism for MetricExporter
s to be registered on the SDK.
See the "Configuration" section of the MeterProvider
.
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).
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? |
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. |
I agree the specification is obtuse here. See this section on Pull-based exporters:
|
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:
The way this current PR is implemented removes explicit wording for (1). |
@jsuereth, makes sense. WDYT of 6d3a25a + c538b91?
@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 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm removing my request for changes on this PR, However, I'm not supportive of the change overall.
- While this removes
ForceFlush
fromMetricReader
, it actually creates a tight coupling between the ownership model ofMetricReader
,MetricExporter
andMeterProvider
. These three things need to fully understand each other, where the previous wording of the spec basically only neededMeterProvider<->MetricReader
andMetricReader<->MetricExporter
. - 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.
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 |
Supersedes (prior art) #3563
Why: #3563 (comment)
By @reyang
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 forMetricProvider
andPush Metric Exporters
.Take notice that MetricReader operations does not define
ForceFlush
. OTel .NET SDK reader's DO NOT implementForceFlush
. As far as I understand theMetricReader
can still haveForceFlush
, but it is a detail which is left to the SDK implementation how metrics SDK accomplishes the goal ofMetricProvider.ForceFlush
.Related (currently blocked) PR in Go SDK: open-telemetry/opentelemetry-go#4375