Change Shutdown() and ForceFlush() to return bool for traces #419

ThomsonTan · 2020-12-03T23:07:03Z

This resolves #400. The default timeout is also set to std::chrono::microseconds::max() as the spec requires Shutdown() Shutdown should not block indefinitely.

codecov · 2020-12-03T23:15:53Z

Codecov Report

Merging #419 (4fbaccb) into master (6355819) will decrease coverage by 0.02%.
The diff coverage is 88.57%.

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
- Coverage   94.53%   94.50%   -0.03%     
==========================================
  Files         179      178       -1     
  Lines        7702     7701       -1     
==========================================
- Hits         7281     7278       -3     
- Misses        421      423       +2

Impacted Files	Coverage Δ
...de/opentelemetry/exporters/ostream/span_exporter.h	`96.29% <ø> (ø)`
sdk/include/opentelemetry/sdk/trace/exporter.h	`100.00% <ø> (ø)`
sdk/include/opentelemetry/sdk/trace/processor.h	`100.00% <ø> (ø)`
.../include/opentelemetry/sdk/trace/tracer_provider.h	`100.00% <ø> (ø)`
...include/opentelemetry/sdk/trace/simple_processor.h	`88.23% <60.00%> (+0.73%)`	⬆️
sdk/src/trace/batch_span_processor.cc	`92.59% <66.66%> (-1.16%)`	⬇️
...lemetry/exporters/memory/in_memory_span_exporter.h	`100.00% <100.00%> (ø)`
exporters/ostream/src/span_exporter.cc	`89.13% <100.00%> (ø)`
exporters/ostream/test/ostream_span_test.cc	`100.00% <100.00%> (ø)`
...nclude/opentelemetry/ext/zpages/tracez_processor.h	`100.00% <100.00%> (ø)`
... and 6 more

lalitb · 2020-12-04T13:05:44Z

docs/cpp-ostream-exporter-design.md

@@ -111,9 +111,10 @@ public:
        return sdktrace::ExportResult::kSuccess;
    }

-    void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept
+    bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(-1)) noexcept


nit: should it be std::chrono::microseconds::max() ?

lalitb · 2020-12-04T14:02:50Z

exporters/otlp/include/opentelemetry/exporters/otlp/otlp_exporter.h

   */
-  void Shutdown(
-      std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept override{};
+  bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept override{


I know timeout is not used here, but we are setting it's default as max at some places and 0 at other (including here). Is it intended ?

+1 We shouldn't specify defaults in overridden methods. AFAIK it should pull the default from the base type.

Seems overrider doesn't acquire the defaults from base class.

8.3.6.10:
A virtual function call (10.3) uses the default arguments in the declaration of the virtual function determined by the static type of the pointer or reference denoting the object. An overriding function in a derived class does not acquire default arguments from the function it overrides. Example:

ext/include/opentelemetry/ext/zpages/tracez_processor.h

jsuereth

+1 to returning bool, looks like one syntax error is breaking tests.

exporters/otlp/include/opentelemetry/exporters/otlp/otlp_exporter.h

jsuereth · 2020-12-04T16:18:54Z

exporters/otlp/include/opentelemetry/exporters/otlp/otlp_exporter.h

   */
-  void Shutdown(
-      std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept override{};
+  bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept override{


+1 We shouldn't specify defaults in overridden methods. AFAIK it should pull the default from the base type.

lalitb

LGTM

alolita · 2020-12-07T23:35:55Z

Triaged Dec 7 2020: Need another reviewer to complete review. @jsuereth can you please take a look?

pyohannes · 2020-12-08T00:24:34Z

Approved, but before merging we should have a look why the Bazel asan test fails (maybe it's just a temporary issue).

lalitb · 2020-12-09T18:23:50Z

Approved, but before merging we should have a look why the Bazel asan test fails (maybe it's just a temporary issue).

@ThomsonTan Do you think if error in bazel asan test is related to this PR. Seems its failing consistently:
/usr/include/c++/7/chrono:176:38: runtime error: signed integer overflow: 9223372036854775807 * 1000 cannot be represented in type 'long int'

ThomsonTan · 2020-12-09T19:25:52Z

Approved, but before merging we should have a look why the Bazel asan test fails (maybe it's just a temporary issue).

@ThomsonTan Do you think if error in bazel asan test is related to this PR. Seems its failing consistently:
/usr/include/c++/7/chrono:176:38: runtime error: signed integer overflow: 9223372036854775807 * 1000 cannot be represented in type 'long int'

Looking at ASAN the failure.

ThomsonTan · 2020-12-09T20:13:41Z

The ASAN failure was caused by assigned std::chrono::milliseconds to std::chrono::microseconds. Fixed in the latest commit.

lalitb · 2020-12-10T11:38:26Z

The ASAN failure was caused by assigned std::chrono::milliseconds to std::chrono::microseconds. Fixed in the latest commit.

Thanks @ThomsonTan . @jsuereth its blocked for your review now : )

Pending for more than 4 days.

…lemetry#419)

Change Shutdown and ForceFlush to return bool for traces

b781d40

ThomsonTan requested a review from a team December 3, 2020 23:07

Merge branch 'master' into ShutdownRetBool

9ee0a2d

lalitb reviewed Dec 4, 2020

View reviewed changes

jsuereth previously requested changes Dec 4, 2020

View reviewed changes

ThomsonTan added 3 commits December 4, 2020 10:32

Convert all default timeout value to max()

5e9e9da

Revert unexpected clang-format

c7515c8

Revert line-end to Unix

97223ad

lalitb approved these changes Dec 7, 2020

View reviewed changes

pyohannes approved these changes Dec 8, 2020

View reviewed changes

Merge branch 'master' into ShutdownRetBool

e93a403

Fix unnecessary conversion from milliseconds to microseconds

7d4f99c

lalitb requested a review from jsuereth December 10, 2020 11:37

Merge branch 'master' into ShutdownRetBool

4fbaccb

lalitb merged commit 68e510c into open-telemetry:master Dec 11, 2020

kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 12, 2020

Change Shutdown() and ForceFlush() to return bool for traces (open-te…

fb92b77

…lemetry#419)

kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 12, 2020

Change Shutdown() and ForceFlush() to return bool for traces (open-te…

c855a64

…lemetry#419)

lalitb mentioned this pull request Jan 13, 2021

Define default timeout for all processor and exporter interfaces #418

Closed

ThomsonTan deleted the ShutdownRetBool branch September 22, 2021 04:12

loong-hy mentioned this pull request Sep 5, 2024

[BUILD] add loongarch info #3052

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Shutdown() and ForceFlush() to return bool for traces #419

Change Shutdown() and ForceFlush() to return bool for traces #419

ThomsonTan commented Dec 3, 2020

codecov bot commented Dec 3, 2020 •

edited

Loading

lalitb Dec 4, 2020

lalitb Dec 4, 2020 •

edited

Loading

jsuereth Dec 4, 2020

ThomsonTan Dec 4, 2020

jsuereth left a comment

jsuereth Dec 4, 2020

lalitb left a comment

alolita commented Dec 7, 2020

pyohannes commented Dec 8, 2020

lalitb commented Dec 9, 2020

ThomsonTan commented Dec 9, 2020 •

edited

Loading

ThomsonTan commented Dec 9, 2020

lalitb commented Dec 10, 2020

Change Shutdown() and ForceFlush() to return bool for traces #419

Change Shutdown() and ForceFlush() to return bool for traces #419

Conversation

ThomsonTan commented Dec 3, 2020

codecov bot commented Dec 3, 2020 • edited Loading

Codecov Report

lalitb Dec 4, 2020

Choose a reason for hiding this comment

lalitb Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

jsuereth Dec 4, 2020

Choose a reason for hiding this comment

ThomsonTan Dec 4, 2020

Choose a reason for hiding this comment

jsuereth left a comment

Choose a reason for hiding this comment

jsuereth Dec 4, 2020

Choose a reason for hiding this comment

lalitb left a comment

Choose a reason for hiding this comment

alolita commented Dec 7, 2020

pyohannes commented Dec 8, 2020

lalitb commented Dec 9, 2020

ThomsonTan commented Dec 9, 2020 • edited Loading

ThomsonTan commented Dec 9, 2020

lalitb commented Dec 10, 2020

codecov bot commented Dec 3, 2020 •

edited

Loading

lalitb Dec 4, 2020 •

edited

Loading

ThomsonTan commented Dec 9, 2020 •

edited

Loading