[SDK] Implement Forceflush for Periodic Metric Reader by lalitb · Pull Request #2064 · open-telemetry/opentelemetry-cpp

lalitb · 2023-03-16T21:47:25Z

Fixes #2056

Changes

As of now, Periodic Metric reader tries to perform one last collection and export during Shutdown. And this fetch fails with warning that the Shutdown is already in progress.

The correct approach should be NOT to do this fetch during shutdown, instead implement PeriodicMetricReader::ForceFlush() method to perform the fetch whenever invoked by application. And then application can just invoke this method before doing the shutdown.

WIP as need to add the unit-tests for this.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

…-cpp into periodic-forceflush

codecov · 2023-03-16T22:48:45Z

Codecov Report

Merging #2064 (1b1bce9) into main (380f0f2) will increase coverage by 0.02%.
The diff coverage is 81.40%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2064      +/-   ##
==========================================
+ Coverage   87.17%   87.19%   +0.02%     
==========================================
  Files         166      166              
  Lines        4746     4784      +38     
==========================================
+ Hits         4137     4171      +34     
- Misses        609      613       +4

Impacted Files	Coverage Δ
...metrics/export/periodic_exporting_metric_reader.cc	`79.35% <81.40%> (+5.28%)`	⬆️

... and 1 file with indirect coverage changes

lalitb · 2023-03-21T00:30:22Z

This is ready for review now.

…-cpp into periodic-forceflush

lalitb · 2023-03-21T00:48:52Z

@owent - Once you have time, can you shortly look into the PeriodicMetricReader::ForceFlush(), as the method is inspired from the batch span/log processor method, and you know that part of code better :)

owent · 2023-03-21T01:58:22Z

+  if (result)
+  {
+
+    result = exporter_->ForceFlush(timeout);


Is it better to use timeout_steady if it's greater than zero here? We have already wait some time before and the left timeout should be less.

Good point. Have changed it to use timeout_steady now.

owent · 2023-03-22T06:32:19Z


  is_force_flush_notified_.store(false, std::memory_order_release);
+
+  if (timeout == std::chrono::steady_clock::duration::zero())


Do we need remove this if branch?It does nothing.

yes removed thanks.

owent · 2023-03-22T06:34:22Z

+  if (timeout_steady <= std::chrono::steady_clock::duration::zero())
+  {
+    // forceflush timeout, exporter force-flush won't be triggered.
+    result = false;


Do we need reset result to false here?I think result will keep false and break the loop above when timeout_steady <= std::chrono::steady_clock::duration::zero().
And if result is true and timeout_steady <= std::chrono::steady_clock::duration::zero(), result will also be set to false below.

Yes the while loop for wait_for will break if timeout_steady <= std::chrono::steady_clock::duration::zero() with result as false. Have removed the check. Thanks for noticing.

owent

LGTM now, thanks.

ThomsonTan · 2023-03-24T23:09:01Z

+    cv_.wait_for(lk, remaining_wait_interval_ms, [this]() {
+      if (is_force_wakeup_background_worker_.load(std::memory_order_acquire))
+      {
+        is_force_wakeup_background_worker_.store(false, std::memory_order_release);


Can store fail then false should be returned?

I don't know if it can ever fail. It's atomic assignment operation so should eventually be successful. Also, this method returns void, not sure how to get any failure status from this method.
Let me know if I am missing something :)

ThomsonTan · 2023-03-24T23:10:04Z

+        is_force_wakeup_background_worker_.store(false, std::memory_order_release);
+        return true;
+      }
+      if (IsShutdown())


nit: return IsShutdown();.

fixed. thanks

ThomsonTan · 2023-03-25T06:16:15Z

+    timeout_steady -= std::chrono::steady_clock::now() - start_timepoint;
+  }
+
+  // If it will be already signaled, we must wait util notified.


Suggested change

// If it will be already signaled, we must wait util notified.

// If it will be already signaled, we must wait until notified.

fixed. thanks.

…elemetry#2064)

* commit '7887d32da60f54984a597abccbb0c883f3a51649': (82 commits) [RELEASE] Release version 1.9.0 (open-telemetry#2091) Use sdk_start_ts for MetricData start_ts for instruments having cumulative aggregation temporality. (open-telemetry#2086) [SEMANTIC CONVENTIONS] Upgrade to version 1.20.0 (open-telemetry#2088) [EXPORTER] Add OTLP HTTP SSL support (open-telemetry#1793) Make Windows build environment parallel (open-telemetry#2080) make some hints (open-telemetry#2078) Make some targets parallel in CI pipeline (open-telemetry#2076) [Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-telemetry#2064) Upgraded semantic conventions to 1.19.0 (open-telemetry#2017) Bump actions/stale from 7 to 8 (open-telemetry#2070) Include directory path added for Zipkin exporter example (open-telemetry#2069) Ignore more warning of generated protobuf files than not included in `-Wall` and `-Wextra` (open-telemetry#2067) Add `ForceFlush` for all `LogRecordExporter`s and `SpanExporter`s. (open-telemetry#2000) Remove unused 'alerting' section from prometheus.yml in examples (open-telemetry#2055) Clean warnings in ETW exporters (open-telemetry#2063) Fix default value of `OPENTELEMETRY_INSTALL_default`. (open-telemetry#2062) [EXPORTER] GRPC endpoint scheme should take precedence over OTEL_EXPORTER_OTLP_TRACES_INSECURE (open-telemetry#2060) Fix view names in Prometheus example (open-telemetry#2034) Fix some docs typo (open-telemetry#2057) Checking indices before dereference (open-telemetry#2040) ... # Conflicts: # exporters/ostream/CMakeLists.txt # sdk/src/metrics/state/metric_collector.cc # sdk/src/metrics/state/temporal_metric_storage.cc

lalitb and others added 2 commits March 16, 2023 09:26

fix

17a70e9

fix

85fd20a

lalitb requested a review from a team March 16, 2023 21:47

lalitb added 3 commits March 16, 2023 14:48

Merge branch 'main' into periodic-forceflush

1e96f20

fix

2e24554

Merge branch 'periodic-forceflush' of github.com:lalitb/opentelemetry…

df46084

…-cpp into periodic-forceflush

lalitb changed the title ~~[WIP] Implement Forceflush operation for Periodic Metric Reader~~ [WIP] Implement Forceflush for Periodic Metric Reader Mar 17, 2023

lalitb and others added 2 commits March 20, 2023 17:22

fix

57dac9c

Merge branch 'main' into periodic-forceflush

fdaf848

lalitb changed the title ~~[WIP] Implement Forceflush for Periodic Metric Reader~~ [Metrics SDK] Implement Forceflush for Periodic Metric Reader Mar 21, 2023

lalitb requested review from ThomsonTan and esigo March 21, 2023 00:30

lalitb added 3 commits March 20, 2023 17:33

spacing

9ff3d23

spacing

94315f3

Merge branch 'periodic-forceflush' of github.com:lalitb/opentelemetry…

3a86962

…-cpp into periodic-forceflush

owent reviewed Mar 21, 2023

View reviewed changes

fix

5f9c783

owent reviewed Mar 21, 2023

View reviewed changes

Comment thread sdk/src/metrics/export/periodic_exporting_metric_reader.cc Outdated

fix comments

5973ec7

owent reviewed Mar 22, 2023

View reviewed changes

lalitb added 2 commits March 23, 2023 14:11

fix

ea5ca54

Merge branch 'main' into periodic-forceflush

450cd97

owent approved these changes Mar 24, 2023

View reviewed changes

ThomsonTan reviewed Mar 24, 2023

View reviewed changes

ThomsonTan reviewed Mar 25, 2023

View reviewed changes

lalitb and others added 2 commits March 24, 2023 23:28

review comments

8a3eb55

Merge branch 'main' into periodic-forceflush

1b1bce9

ThomsonTan approved these changes Mar 27, 2023

View reviewed changes

esigo added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Mar 27, 2023

lalitb merged commit bd7a3c7 into open-telemetry:main Mar 27, 2023

cngzhnp pushed a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Mar 27, 2023

[Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-t…

1747804

…elemetry#2064)

cngzhnp pushed a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Mar 28, 2023

[Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-t…

43a1a61

…elemetry#2064)

cngzhnp pushed a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Mar 28, 2023

[Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-t…

4223394

…elemetry#2064)

cngzhnp pushed a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Mar 28, 2023

[Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-t…

f568a0b

…elemetry#2064)

cngzhnp pushed a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Mar 28, 2023

[Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-t…

a1efe35

…elemetry#2064)

cngzhnp pushed a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Mar 28, 2023

[Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-t…

4a7fdb3

…elemetry#2064)

cngzhnp pushed a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Mar 28, 2023

[Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-t…

55a8db1

…elemetry#2064)

marcalff changed the title ~~[Metrics SDK] Implement Forceflush for Periodic Metric Reader~~ [SDK] Implement Forceflush for Periodic Metric Reader Apr 12, 2023

chuanchan1116 mentioned this pull request Apr 19, 2023

opentelemetry-cpp 1.9.0 Homebrew/homebrew-core#128783

Closed

cngzhnp pushed a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Apr 20, 2023

[Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-t…

eb931c4

…elemetry#2064)

lalitb mentioned this pull request Jan 30, 2024

Collect and Export metric data before PeriodicMetricReader shutdown #2516

Closed


		is_force_flush_notified_.store(false, std::memory_order_release);

		if (timeout == std::chrono::steady_clock::duration::zero())

	// If it will be already signaled, we must wait util notified.
	// If it will be already signaled, we must wait until notified.

Conversation

lalitb commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

codecov Bot commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lalitb commented Mar 21, 2023

Uh oh!

lalitb commented Mar 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lalitb commented Mar 16, 2023 •

edited

Loading

codecov Bot commented Mar 16, 2023 •

edited

Loading