Remove guidance about when users should call ForceFlush from the specification #3620
Conversation
Good point! We have the same issue in other specs, e.g. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#forceflush-1. |
dashpole
force-pushed
the
forceflush_docs
branch
from
5edc774
to
7338434
Compare
Updated the trace and log SDK specs as well. |
specification/logs/sdk.md
Outdated
| such as when using some FaaS providers that may suspend the process after an | ||
| invocation, but before the `LogRecordProcessor` exports the | ||
| emitted `LogRecord`s. | ||
| `ForceFlush` SHOULD be documented with a disclaimer stating that it should only |
There was a problem hiding this comment.
This looks okay to me. Just calling out loud, do we want to remove this line entirely? There are many similar APIs such as file.flush https://learn.microsoft.com/en-us/dotnet/api/system.io.filestream.flush.
I guess it could be helpful to explain the cost/consequence rather than saying "only call me when it is absolutely necessary" (part of the reason is that "necessary" is blurry). e.g. https://github.com/open-telemetry/opentelemetry-dotnet/blob/a808276efaf7e7a13420d993cea903cbff262996/src/OpenTelemetry/Logs/LoggerProviderExtensions.cs#L53-L54
There was a problem hiding this comment.
I am also fine with removal, if we don't think the disclaimer is necessary.
There was a problem hiding this comment.
I personally would vote for removal, maybe let's gather more opinions?
Folks who prefer removal please give 👍, otherwise 👎.
There was a problem hiding this comment.
Updated the PR to remove these sections.
dashpole
force-pushed
the
forceflush_docs
branch
from
e103d3b
to
e5fb8c9
Compare
dashpole
changed the title
dashpole
force-pushed
the
forceflush_docs
branch
from
e5fb8c9
to
db77940
Compare
The normative guidance is not for the intended readers of this document, it is for their users. Having this language here implies that the SDKs need to somehow restrict the use of The practical benefit to removing this is that is removes an at best confusing and at worst unimplementable part of the specification. The original PR changed this to be something the SDK authors could feasibly accomplish, namely have it be documentation for their users. However, it was recommended to remove it entirely. If you think that is a better approach, can you please coordinate with @reyang so we can reach a resolution here. |
|
An alternative would be to do something similar to: |
|
I am OK with adding a documentation requirement, but it might be overkill. I can also see why the current wording with SHOULD is technically wrong. How about changing it to a non-normative "Note:" and using lowercase "should" or wording such as "is only meant for ..."? |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Changes
The spec for metric exporter's ForceFlush method doesn't currently have any guidance for the SDK authors. Instead, it appears to provide guidance to users about when they should call ForceFlush.
This PR removes that guidance because it is not relevant to the specification of the SDK.
As an alternative, we could keep the guidance, but clarify that SDK authors SHOULD document ForceFlush with it. This was rejected in #3620 (review)
Originally raised here: open-telemetry/opentelemetry-go#4359 (review)