Skip to content
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

Add disclaimer to ForceFlush to recommend calling only when necessary #4359

Closed
wants to merge 1 commit into from

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Jul 24, 2023

Part of #3666

Spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#forceflush-1

ForceFlush SHOULD only be called in cases where it is absolutely necessary, such as when using some FaaS providers that may suspend the process after an invocation, but before the exporter exports the completed metrics.

This adds the disclaimer from the specification to our interfaces which have ForceFlush

@dashpole dashpole added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 24, 2023
@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #4359 (a3e1fe1) into main (e26d8bd) will decrease coverage by 0.1%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4359     +/-   ##
=======================================
- Coverage   83.5%   83.4%   -0.1%     
=======================================
  Files        184     184             
  Lines      14383   14383             
=======================================
- Hits       12011   12009      -2     
- Misses      2145    2147      +2     
  Partials     227     227             
Impacted Files Coverage Δ
sdk/metric/reader.go 100.0% <ø> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the specification needs to be updated to make the normative recommendation about having this type of documentation. It cannot recommend to the user in the specification what they should or shouldn't do given they are not the intended audience and the user has not requirement to follow the specification.

@pellared
Copy link
Member

I am not approving because I have upvoted open-telemetry/opentelemetry-specification#3620 (comment) 😉

@dashpole
Copy link
Contributor Author

Converting to draft while we decide if this is necessary

@dashpole dashpole marked this pull request as draft July 26, 2023 15:23
@dashpole
Copy link
Contributor Author

dashpole commented Aug 3, 2023

closing for now. I'll reopen if it is needed.

@dashpole dashpole closed this Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants